[PATCH] D103589: [Format] Fix incorrect pointer detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu created this revision.
Nuu added a project: clang-format.
Nuu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Before:

  void foo() { bar(float(1), a *b); }

After:

  void foo() { bar(float(1), a * b); }

Signed-off-by: Yilong Guo 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103589

Files:
  clang/lib/Format/TokenAnnotator.cpp


Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -399,7 +399,7 @@
 HasMultipleParametersOnALine = true;
   if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
-  !CurrentToken->is(tok::l_brace))
+  !CurrentToken->isOneOf(tok::l_brace, tok::l_paren))
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;


Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -399,7 +399,7 @@
 HasMultipleParametersOnALine = true;
   if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
-  !CurrentToken->is(tok::l_brace))
+  !CurrentToken->isOneOf(tok::l_brace, tok::l_paren))
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103589: [Format] Fix incorrect pointer detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu updated this revision to Diff 349455.
Nuu edited the summary of this revision.
Nuu added a comment.

Add test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103589

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8664,6 +8664,7 @@
 
   verifyIndependentOfContext("MACRO('0' <= c && c <= '9');");
   verifyFormat("void f() { f(float{1}, a * a); }");
+  verifyFormat("void f() { f(float(1), a * a); }");
   // FIXME: Is there a way to make this work?
   // verifyIndependentOfContext("MACRO(A *a);");
   verifyFormat("MACRO(A &B);");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -399,7 +399,7 @@
 HasMultipleParametersOnALine = true;
   if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
-  !CurrentToken->is(tok::l_brace))
+  !CurrentToken->isOneOf(tok::l_brace, tok::l_paren))
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8664,6 +8664,7 @@
 
   verifyIndependentOfContext("MACRO('0' <= c && c <= '9');");
   verifyFormat("void f() { f(float{1}, a * a); }");
+  verifyFormat("void f() { f(float(1), a * a); }");
   // FIXME: Is there a way to make this work?
   // verifyIndependentOfContext("MACRO(A *a);");
   verifyFormat("MACRO(A &B);");
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -399,7 +399,7 @@
 HasMultipleParametersOnALine = true;
   if ((CurrentToken->Previous->isOneOf(tok::kw_const, tok::kw_auto) ||
CurrentToken->Previous->isSimpleTypeSpecifier()) &&
-  !CurrentToken->is(tok::l_brace))
+  !CurrentToken->isOneOf(tok::l_brace, tok::l_paren))
 Contexts.back().IsExpression = false;
   if (CurrentToken->isOneOf(tok::semi, tok::colon)) {
 MightBeObjCForRangeLoop = false;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103589: [Format] Fix incorrect pointer detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

In D103589#2795447 , @curdeius wrote:

> Thanks for looking into this, but please add a test case that demonstrates 
> the bug to `FormatTests.cpp` 
> Also, is there, by any chance, an associated bugzilla ticket?

Thanks for the comment.
I've added a test and linked to the bugzilla ticket 
 in summary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103589

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

[PATCH] D103589: [Format] Fix incorrect pointer detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

In D103589#2796032 , @curdeius wrote:

> LGTM. Thanks for fixing this!
> Do you have commit rights or you need somebody to land it for you?

I don't have commit access. I'd appreciate it if you or someone else could land 
this patch for me :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103589

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

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu created this revision.
Nuu added reviewers: djasper, HazardyKnusperkeks, curdeius, MyDeveloperDay.
Nuu added a project: clang-format.
Nuu requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

https://llvm.org/PR50568

When an overloaded operator is called, its argument must be an
expression.

Before:

  void f() { a.operator()(a *a); }

After:

  void f() { a.operator()(a * a); }

Signed-off-by: Yilong Guo 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D103678

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8761,6 +8761,11 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function)
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev);
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)"
+  // i.e. the operator is called as a member function
+  // then the argument must be an expression
+  bool OperatorCalledAsMemberFunction =
+  Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+  Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
 } else if (Style.Language == FormatStyle::LK_JavaScript &&
(Line.startsWith(Keywords.kw_type, tok::identifier) ||
 Line.startsWith(tok::kw_export, Keywords.kw_type,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8761,6 +8761,11 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function)
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev);
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)"
+  // i.e. the operator is called as a member function
+  // then the argument must be an expression
+  bool OperatorCalledAsMemberFunction =
+  Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+  Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
 } else if (Style.Language == FormatStyle::LK_JavaScript &&
(Line.startsWith(Keywords.kw_type, tok::identifier) ||
 Line.startsWith(tok::kw_export, Keywords.kw_type,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu updated this revision to Diff 350519.
Nuu added a comment.

Add more tests.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103678

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8274,6 +8274,16 @@
   verifyFormat("using A::operator+;");
   verifyFormat("inline A operator^(const A &lhs, const A &rhs) {}\n"
"int i;");
+
+  // Calling an operator as a member function
+  verifyFormat("void f() { a.operator*(); }");
+  verifyFormat("void f() { a.operator*(b & b); }");
+  verifyFormat("void f() { a->operator&(a * b); }");
+  verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
@@ -8761,6 +8771,13 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function)
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
+  verifyFormat("void f() { a.operator()(*a & *a); }");
+  verifyFormat("void f() { a->operator()(*a * *a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev);
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)"
+  // i.e. the operator is called as a member function
+  // then the argument must be an expression
+  bool OperatorCalledAsMemberFunction =
+  Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+  Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
 } else if (Style.Language == FormatStyle::LK_JavaScript &&
(Line.startsWith(Keywords.kw_type, tok::identifier) ||
 Line.startsWith(tok::kw_export, Keywords.kw_type,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8274,6 +8274,16 @@
   verifyFormat("using A::operator+;");
   verifyFormat("inline A operator^(const A &lhs, const A &rhs) {}\n"
"int i;");
+
+  // Calling an operator as a member function
+  verifyFormat("void f() { a.operator*(); }");
+  verifyFormat("void f() { a.operator*(b & b); }");
+  verifyFormat("void f() { a->operator&(a * b); }");
+  verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
@@ -8761,6 +8771,13 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function)
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
+  verifyFormat("void f() { a.operator()(*a & *a); }");
+  verifyFormat("void f() { a->operator()(*a * *a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev);
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)"
+  // i.e. the operator is called as a member function
+  // then the argument must be an expression
+  bool OperatorCalledAsMemberFunction =
+

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu marked an inline comment as done.
Nuu added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:243
+  bool OperatorCalledAsMemberFunction =
+  Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+  Contexts.back().IsExpression = OperatorCalledAsMemberFunction;

MyDeveloperDay wrote:
> feels like we are not testing this situation, please add those tests
I added some in `FormatTest.UnderstandsOverloadedOperators`



Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");

This patch doesn't fix these, i.e. when operators are called as non-member 
functions.
The call sites seem to be marked as function declarations.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103678

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

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

Pre-check failure: the github server is currently down.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103678

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

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu updated this revision to Diff 350756.
Nuu added a comment.

Minor updates.


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

https://reviews.llvm.org/D103678

Files:
  clang/lib/Format/TokenAnnotator.cpp
  clang/unittests/Format/FormatTest.cpp


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8274,6 +8274,16 @@
   verifyFormat("using A::operator+;");
   verifyFormat("inline A operator^(const A &lhs, const A &rhs) {}\n"
"int i;");
+
+  // Calling an operator as a member function.
+  verifyFormat("void f() { a.operator*(); }");
+  verifyFormat("void f() { a.operator*(b & b); }");
+  verifyFormat("void f() { a->operator&(a * b); }");
+  verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+  // TODO: Calling an operator as a non-member function is hard to distinguish.
+  // https://llvm.org/PR50629
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
@@ -8761,6 +8771,13 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function).
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
+  verifyFormat("void f() { a.operator()(*a & *a); }");
+  verifyFormat("void f() { a->operator()(*a * *a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token.
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev && "Expect a kw_operator prior to the OperatorLParen!");
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)",
+  // i.e. the operator is called as a member function,
+  // then the argument must be an expression.
+  bool OperatorCalledAsMemberFunction =
+  Prev->Previous && Prev->Previous->isOneOf(tok::period, tok::arrow);
+  Contexts.back().IsExpression = OperatorCalledAsMemberFunction;
 } else if (Style.Language == FormatStyle::LK_JavaScript &&
(Line.startsWith(Keywords.kw_type, tok::identifier) ||
 Line.startsWith(tok::kw_export, Keywords.kw_type,


Index: clang/unittests/Format/FormatTest.cpp
===
--- clang/unittests/Format/FormatTest.cpp
+++ clang/unittests/Format/FormatTest.cpp
@@ -8274,6 +8274,16 @@
   verifyFormat("using A::operator+;");
   verifyFormat("inline A operator^(const A &lhs, const A &rhs) {}\n"
"int i;");
+
+  // Calling an operator as a member function.
+  verifyFormat("void f() { a.operator*(); }");
+  verifyFormat("void f() { a.operator*(b & b); }");
+  verifyFormat("void f() { a->operator&(a * b); }");
+  verifyFormat("void f() { NS::a.operator+(*b * *b); }");
+  // TODO: Calling an operator as a non-member function is hard to distinguish.
+  // https://llvm.org/PR50629
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
 }
 
 TEST_F(FormatTest, UnderstandsFunctionRefQualification) {
@@ -8761,6 +8771,13 @@
"operator()() && {}");
   verifyGoogleFormat("template \n"
  "auto x() & -> int {}");
+
+  // Should be binary operators when used as an argument expression (overloaded
+  // operator invoked as a member function).
+  verifyFormat("void f() { a.operator()(a * a); }");
+  verifyFormat("void f() { a->operator()(a & a); }");
+  verifyFormat("void f() { a.operator()(*a & *a); }");
+  verifyFormat("void f() { a->operator()(*a * *a); }");
 }
 
 TEST_F(FormatTest, UnderstandsAttributes) {
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -229,7 +229,19 @@
 }
 
 if (Left->is(TT_OverloadedOperatorLParen)) {
-  Contexts.back().IsExpression = false;
+  // Find the previous kw_operator token.
+  FormatToken *Prev = Left;
+  while (!Prev->is(tok::kw_operator)) {
+Prev = Prev->Previous;
+assert(Prev && "Expect a kw_operator prior to the OperatorLParen!");
+  }
+
+  // If faced with "a.operator*(argument)" or "a->operator*(argument)",
+  // i.e. the operator

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:236
+Prev = Prev->Previous;
+assert(Prev);
+  }

MyDeveloperDay wrote:
> curdeius wrote:
> > MyDeveloperDay wrote:
> > > Do we need to worry about `Prev` ever being null? Does the assert ever 
> > > fire? if not why have it in the first place?
> > > 
> > > We'll crash if it is, do we want to guard against that?
> > Disclaimer: I'm in the pro-assert camp :).
> > In general, I'd preferably keep this assert because people sometimes use 
> > Release+Assertions build. Even more those that report bugs.
> > And it's much nicer and easier to fix  an assertion failure that is well 
> > defined and precisely localised than a less clearer report of a crash.
> > Of course, in a build without assertions, it will crash if `Prev` is null, 
> > but still it will be easier to find the bug when trying to reproduce.
> > 
> > For this specific case, this assertion should never fire because otherwise 
> > it would mean that we detected had `Left` being 
> > `TT_OverloadedOperatorLParen` but there was no corresponding 
> > `tok::kw_operator` (or just `operator`). So it would mean that there's a 
> > bug in (probably) token annotator. So yeah, it falls into "should never 
> > happen" category about which assertions are all about IMO.
> To be honest I'm in the assert and guard as well camp (if such a camp exists 
> ;-)
> 
> I have this discussion regularly with the other developers in my team, the 
> use assert is useless other than for our assertion builds (which in  high 
> performance code, almost no one runs, until there is a problem!)
> 
> The assert triggers in my mind that it MIGHT be null (but generally 
> shouldn't) so in which case we have to guard as well for the rare case, but I 
> like the assert.. but please of the form:
> 
> ```assert(condition && "Some nice string telling me exactly what went 
> wrong")```
> 
> When the assert fires I get a nice error message, not just assert failed with 
> `0`
> 
> https://llvm.org/docs/CodingStandards.html#assert-liberally
> 
@curdeius @MyDeveloperDay Thanks for the explanations. I've added an error 
message.



Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");

MyDeveloperDay wrote:
> curdeius wrote:
> > curdeius wrote:
> > > Nuu wrote:
> > > > This patch doesn't fix these, i.e. when operators are called as 
> > > > non-member functions.
> > > > The call sites seem to be marked as function declarations.
> > > Is there a bug report on that? If so, please add a link to the comment. 
> > > If no, could you please create one?
> > > Or is it really something that will never be possible to distinguish 
> > > (hmm, this would be strange)?
> > Nit: full-stop.
> Might be nice just to fix these as well, this is kind of what happens with 
> clang-format development... you fix one thing we try to make you fix 
> everything else around it, otherwise who is going to understand it better 
> than you! ;-)
> 
> We do appreciate your patch!!
@curdeius 

> Is there a bug report on that? If so, please add a link to the comment. If 
> no, could you please create one?

No. I've created one and added the link in the comment!

> Or is it really something that will never be possible to distinguish (hmm, 
> this would be strange)?

I think it should be distinguishable, at least there should be no return type 
before the operator in the context of non-member function invocation. I'd like 
to fix this in a separate patch, later. I need some time to figure it out :)



Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");

Nuu wrote:
> MyDeveloperDay wrote:
> > curdeius wrote:
> > > curdeius wrote:
> > > > Nuu wrote:
> > > > > This patch doesn't fix these, i.e. when operators are called as 
> > > > > non-member functions.
> > > > > The call sites seem to be marked as function declarations.
> > > > Is there a bug report on that? If so, please add a link to the comment. 
> > > > If no, could you please create one?
> > > > Or is it really something that will never be possible to distinguish 
> > > > (hmm, this would be strange)?
> > > Nit: full-stop.
> > Might be nice just to fix these as well, this is kind of what happens with 
> > clang-format development... you fix one thing we try to make you fix 
> > everything else around it, otherwise who is going to understand it better 
> > than you! ;-)
> > 
> > We do appreciate your patch!!
> @curdeius 
> 
> > Is there a bug report on that? If so, please ad

[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

In D103678#2811302 , 
@HazardyKnusperkeks wrote:

> Do you need some one to commit this? If yes please state name and email, some 
> one will chime in to commit it.

Yes, please someone help commit this.

Name: Yilong Guo
Email: yilong@intel.com

Thanks in advance!


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

https://reviews.llvm.org/D103678

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

[PATCH] D114082: [WIP] Normalize String Attributes

[Yilong Guo via Phabricator via cfe-commits]

Nuu added a comment.

Thanks for this. I planned something similar to turn string literal typo errors 
into compile errors as well.
The typos are really hard to identify :-P

Some suggestions on using tablegen.




Comment at: llvm/include/llvm/IR/Attributes.h:977-1133
+constexpr llvm::AttributeKey
+AmdgpuFlatWorkGroupSizeAttr("amdgpu-flat-work-group-size");
+constexpr llvm::AttributeKey AmdgpuIeeeAttr("amdgpu-ieee");
+constexpr llvm::AttributeKey
+AmdgpuImplicitargNumBytesAttr("amdgpu-implicitarg-num-bytes");
+constexpr llvm::AttributeKey AmdgpuNumSgprAttr("amdgpu-num-sgpr");
+constexpr llvm::AttributeKey AmdgpuNumVgprAttr("amdgpu-num-vgpr");

I think we can simplify these definitions via "Attributes.inc", by adding one 
more class tablegen class and defining the symbolic -> display name mapping in 
"Attributes.td".


```
// llvm/utils/TableGen/Attributes.cpp emitTargetsIndependentNames()
Emit({"EnumAttr", "TypeAttr", "IntAttr"}, "ATTRIBUTE_ENUM");
Emit({"StrBoolAttr"}, "ATTRIBUTE_STRBOOL");
Emit({"StrKeyAttr"}, "ATTRIBUTE_KEY");
```



Comment at: llvm/include/llvm/IR/Attributes.td:43
 class StrBoolAttr : Attr;
 
 /// Target-independent enum attributes.

Define a dedicated tablegen class for AttributeKey generation.



Comment at: llvm/include/llvm/IR/Attributes.td:303
 def UseSampleProfile : StrBoolAttr<"use-sample-profile">;
 
 class CompatRule {

Map symbolic names with string literals.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D114082

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

[PATCH] D114394: Compile-time computation of string attribute hashes

[Yilong Guo via Phabricator via cfe-commits]

Nuu added inline comments.



Comment at: llvm/include/llvm/IR/Attributes.h:54
+size_(HashedS.size()) {
+assert(hash_ == hasher(s.data(), s.size()) && "consistent hashing");
+  }




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

https://reviews.llvm.org/D114394

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

[PATCH] D153798: [clang-format] Correctly annotate operator free function call

[Yilong Guo via Phabricator via cfe-commits]

Nuu accepted this revision.
Nuu added a comment.
This revision is now accepted and ready to land.

Thanks, LGTM!




Comment at: clang/lib/Format/TokenAnnotator.cpp:316
+  // line can't be a declaration.
   bool OperatorCalledAsMemberFunction =
+  (Prev->Previous &&

minor. Probably better to rename it to something like `IsOperatorCallSite`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153798

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