[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb abandoned this revision.
cjdb added a comment.
Herald added a project: All.

I think there was a clang-tidy patch that handled this.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added inline comments.



Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1553
+  "%select{references|destructors|block pointers|ref-qualified member 
functions}2">,
+  InGroup>;
+

@cjdb: I suggest splitting this diagnostic up, mainly for sanity's sake (it 
currently has 4x2x4 = 32 possible outputs, which is Too Much) but also to 
improve greppability (it took me five tries to find the source of the message 
"when declaring destructors" in this PR). I would do one diagnostic `"use 
'%{&|&&}0' when declaring %{lvalue|rvalue}1 %{references|ref-qualified member 
functions}"`; another diagnostic `"use '~' when declaring destructors"`; and a 
third `"use '^' when declaring block pointers"`.

@aaron.ballman wrote:
> One question I have about both declarations and expressions are whether we 
> have an appetite to diagnose overloaded operators or not. Personally, I think 
> it'd be reasonable to diagnose something like `foo->operator bitand();` or 
> `operator not_eq(A, B);` as expressions, but not reasonable to diagnose the 
> declaration of the overloaded operators using alternative tokens.

AIUI, @cjdb is deferring all diagnostics related to //expressions// into a 
later PR, and I think that's reasonable. There's a subtlety to your example I 
can't tell if you intended: `foo->operator bitand()` is clearly wrong because 
the unary operator `&` being invoked there is not `bitand`; it's address-of! 
`foo->operator bitand()` will presumably fall into the same category as 
`foo->compl Foo()`. Whereas there's nothing so wrong with `foo->operator 
not_eq(bar)` or `operator not_eq(foo, bar)`. 

However, all that has at least two adjacent issues that I know are on @cjdb's 
radar: (1) `auto transformed = foo bitor std::views::transform(bar)` is 
"wrong", and in fact (2) //any// use of alternative tokens other than `and`, 
`or`, `not` is unstylish. If you diagnose //all// usage of `bitand`, `bitor`, 
`compl`, etc., then you don't get any of these subtle problems, because 
`&&/||/!` only have one meaning each (except for rvalue references, which is 
handled in this PR; and GCC-extension `&`, which should be easy to catch).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D107292#2943101 , @cjdb wrote:

> In D107292#2939521 , @aaron.ballman 
> wrote:
>
>> One question I have about both declarations and expressions are whether we 
>> have an appetite to diagnose overloaded operators or not. Personally, I 
>> think it'd be reasonable to diagnose something like `foo->operator 
>> bitand();` or `operator not_eq(A, B);` as expressions, but not reasonable to 
>> diagnose the declaration of the overloaded operators using alternative 
>> tokens.
>
> I agree that `bool operator and(T, T);` shouldn't be diagnosed on (and this 
> patch's clang-tidy sibling will one day also diagnose that, but it's way off).
>
> I think that `foo->operator bitand()` and `operator not_eq(expr1, expr2)` 
> should only diagnose if `foo->operator&()` and `operator!=(expr1, expr2)` are 
> diagnosed, //and// I think that should be a separate warning (I'm not saying 
> that's a good or bad thing to do yet: let me sleep on that). I might be 
> misunderstanding your intention though.

The situation I was thinking of was where the the declaration is for an 
`operator&()` function but the expression is calling `operator bitand()` (or 
vice versa), but 1) I don't feel strongly it needs to be diagnosed, mostly just 
exploring the shape of the diagnostic, and 2) I'd be fine if it was handled 
under a separate flag at some later date.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

In D107292#2939521 , @aaron.ballman 
wrote:

> In D107292#2923261 , @cjdb wrote:
>
>> Patch 2: expressions
>>
>> xor {}
>> bitand x // warning will suggest std::addressof in C++ land
>> and label
>
> An additional expression to cover, not that I think anyone would be this 
> awful by accident, is: `foo->compl Foo(); // Pseudo-destructor call`

Nice catch!

> One question I have about both declarations and expressions are whether we 
> have an appetite to diagnose overloaded operators or not. Personally, I think 
> it'd be reasonable to diagnose something like `foo->operator bitand();` or 
> `operator not_eq(A, B);` as expressions, but not reasonable to diagnose the 
> declaration of the overloaded operators using alternative tokens.

I agree that `bool operator and(T, T);` shouldn't be diagnosed on (and this 
patch's clang-tidy sibling will one day also diagnose that, but it's way off).

I think that `foo->operator bitand()` and `operator not_eq(expr1, expr2)` 
should only diagnose if `foo->operator&()` and `operator!=(expr1, expr2)` are 
diagnosed, //and// I think that should be a separate warning (I'm not saying 
that's a good or bad thing to do yet: let me sleep on that). I might be 
misunderstanding your intention though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D107292#2923261 , @cjdb wrote:

> Patch 2: expressions
>
> xor {}
> bitand x // warning will suggest std::addressof in C++ land
> and label

An additional expression to cover, not that I think anyone would be this awful 
by accident, is: `foo->compl Foo(); // Pseudo-destructor call`

One question I have about both declarations and expressions are whether we have 
an appetite to diagnose overloaded operators or not. Personally, I think it'd 
be reasonable to diagnose something like `foo->operator bitand();` or `operator 
not_eq(A, B);` as expressions, but not reasonable to diagnose the declaration 
of the overloaded operators using alternative tokens.




Comment at: clang/lib/Parse/ParseDecl.cpp:5830-5832
+constexpr int Caret = 3;
+constexpr int Empty = 2;
+constexpr int BlockPointers = 2;

We typically either use comments for one-off uses, or we hoist this sort of 
thing to an enumeration associated with the diagnostic if we think the 
diagnostic will be used in enough places to warrant it. I think we should 
probably stick to comments for this one (e.g., `Diag(Loc, diag::whatever) << 
/*frobble*/1;`, but if you think a hoisted enum is a better approach I won't 
argue it.

(Same comment applies elsewhere in the patch.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb added a comment.

Ping @aaron.ballman


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D107292: [clang] adds warning to alert user when they use alternative tokens in declarations

[Christopher Di Bella via Phabricator via cfe-commits]

cjdb updated this revision to Diff 363927.
cjdb retitled this revision from "[clang] adds warning to alert user when they 
use alternative tokens as references" to "[clang] adds warning to alert user 
when they use alternative tokens in declarations".
cjdb edited the summary of this revision.
cjdb added a comment.

extends functionality to account for destructors, member functions, and block 
pointers


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D107292/new/

https://reviews.llvm.org/D107292

Files:
  clang/include/clang/Basic/DiagnosticParseKinds.td
  clang/lib/Parse/ParseDecl.cpp
  clang/lib/Parse/ParseExprCXX.cpp
  clang/test/Parser/cxx-decl.cpp
  clang/test/Parser/cxx0x-decl.cpp
  clang/test/Parser/warn-declare-references-with-symbols.cpp

Index: clang/test/Parser/warn-declare-references-with-symbols.cpp
===
--- /dev/null
+++ clang/test/Parser/warn-declare-references-with-symbols.cpp
@@ -0,0 +1,130 @@
+// RUN: %clang_cc1 -std=c++98 -fblocks -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++98 -fblocks -fsyntax-only -Wdeclare-with-symbols -verify %s
+// RUN: %clang_cc1 -std=c++11 -fblocks -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++11 -fblocks -fsyntax-only -Wdeclare-with-symbols -verify %s
+// RUN: %clang_cc1 -std=c++14 -fblocks -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++14 -fblocks -fsyntax-only -Wdeclare-with-symbols -verify %s
+// RUN: %clang_cc1 -std=c++20 -fblocks -fsyntax-only -verify %s
+// RUN: %clang_cc1 -std=c++20 -fblocks -fsyntax-only -Wdeclare-with-symbols -verify %s
+
+int i = 0;
+
+int bitand lr1 = i; // expected-warning{{use '&' when declaring lvalue references}}
+int  = i; // no warning
+
+template
+void f(T bitand); // expected-warning{{use '&' when declaring lvalue references}}
+
+template
+void f(T &); // no warning
+
+int(xor bp1)(); // expected-warning{{use '^' when declaring block pointers}}
+int(^bp2)();
+
+struct S1 {
+  compl S1(); // expected-warning{{use '~' when declaring destructors}}
+};
+
+struct S2 {
+  ~S2();
+};
+
+S2::compl S2() // expected-warning{{use '~' when declaring destructors}}
+{}
+
+struct S3 {
+  ~S3(); // no warning
+};
+
+struct S4 {
+  ~S4();
+};
+
+S4::~S4() // no warning
+{}
+
+#if __cplusplus >= 201103L
+int and rr1 = 0; // expected-warning{{use '&&' when declaring rvalue references}}
+int & = 0; // no warning
+
+using bad_block_ptr = int(xor)(); // expected-warning{{use '^' when declaring block pointers}}
+using good_block_ptr = int(^)();
+
+using bad_lr = int bitand; // expected-warning{{use '&' when declaring lvalue references}}
+using good_lr = int &; // no warning
+
+using bad_rr = int and; // expected-warning{{use '&&' when declaring rvalue references}}
+using good_rr = int &&; // no warning
+
+auto and fr1 = i; // expected-warning{{use '&&' when declaring rvalue references}}
+auto & = i; // no warning
+
+auto and fr3 = 0; // expected-warning{{use '&&' when declaring rvalue references}}
+auto & = 0; // no warning
+
+template
+void f(T and); // expected-warning{{use '&&' when declaring rvalue references}}
+
+template
+void f(T &&); // no warning
+
+struct S5 {
+  void f() bitand;
+  // expected-warning@-1{{use '&' when declaring lvalue ref-qualified member functions}}
+  void f() const bitand;
+  // expected-warning@-1{{use '&' when declaring lvalue ref-qualified member functions}}
+  void f() volatile bitand;
+  // expected-warning@-1{{use '&' when declaring lvalue ref-qualified member functions}}
+  void f() const volatile bitand;
+  // expected-warning@-1{{use '&' when declaring lvalue ref-qualified member functions}}
+  void f() and;
+  // expected-warning@-1{{use '&&' when declaring rvalue ref-qualified member functions}}
+  void f() const and;
+  // expected-warning@-1{{use '&&' when declaring rvalue ref-qualified member functions}}
+  void f() volatile and;
+  // expected-warning@-1{{use '&&' when declaring rvalue ref-qualified member functions}}
+  void f() const volatile and;
+  // expected-warning@-1{{use '&&' when declaring rvalue ref-qualified member functions}}
+};
+
+struct S6 {
+  void f() &; // no warning
+  void f() const &; // no warning
+  void f() volatile &; // no warning
+  void f() const volatile &; // no warning
+  void f() &&; // no warning
+  void f() const &&; // no warning
+  void f() volatile &&; // no warning
+  void f() const volatile &&; // no warning
+};
+#endif // __cplusplus > 201103L
+
+#if __cplusplus >= 201402L
+template
+T bitand lr3 = i; // expected-warning{{use '&' when declaring lvalue references}}
+
+template
+T  = i; // no warning
+
+template
+T and rr3 = i; // expected-warning{{use '&&' when declaring rvalue references}}
+
+template
+T & = i; // no warning
+#endif // __cplusplus >= 201402L
+
+#if __cplusplus >= 202002L
+template
+concept C1 = requires(T bitand x) { x; };
+// expected-warning@-1{{use '&' when declaring lvalue references}}
+
+template
+concept C2 =