[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz abandoned this revision.
tamas.petz added a comment.

In D75364#1966818 , @MyDeveloperDay 
wrote:

> In D75364#1966743 , @tamas.petz 
> wrote:
>
> > Wow, I have missed that configuration option.
> >  I will try it, I assume it should work.
> >
> > Looks like this change should be abandoned.
>
>
> To be honest, I forget what we've got too! ;-) I was just writing a reply 
> that said "how about adding an option with a list of type macros", I was 
> looking for an example of other places we do that, and stumbled on it.


: ) I am now abandoning this change.
Thank you all for you comments.


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

In D75364#1966743 , @tamas.petz wrote:

> Wow, I have missed that configuration option.
>  I will try it, I assume it should work.
>
> Looks like this change should be abandoned.


To be honest, I forget what we've got too! ;-) I was just writing a reply that 
said "how about adding an option with a list of type macros", I was looking for 
an example of other places we do that, and stumbled on it.


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz added a comment.

Wow, I have missed that configuration option.
I will try it, I assume it should work.

Looks like this change should be abandoned.


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added a comment.

Did you try putting 'ElfW and 'M' in the list of TypenameMacros?

  ---
  Language: Cpp
  BasedOnStyle: LLVM
  TypenameMacros: ['ElfW']
  PointerAlignment: Left



  const ElfW(Addr)* foo(ElfW(Addr)* addr);
  const M(Addr) * foo(M(Addr) * addr);



  $ clang-format macros.cpp
  const ElfW(Addr)* foo(ElfW(Addr)* addr);
  const M(Addr) * foo(M(Addr) * addr);

Simply extend the list to include the macros you need

  TypenameMacros: ['ElfW','M']



  $ clang-format macros.cpp
  const ElfW(Addr)* foo(ElfW(Addr)* addr);
  const M(Addr)* foo(M(Addr)* addr);

F11683170: image.png 


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz marked 2 inline comments as done.
tamas.petz added a comment.

@all, thank you for the review so far.

The case I am trying to handle is way too ambiguous.
IMHO looking at tokens only is not going to lead to a "perfect" formatter.
The case I am trying to handle is quite common but it can lead to incorrect 
formatting in other cases.

I had a weak hope that passing all tests is "safe enough" but it is 
unfortunately not.

Should we just note that cases similar to the one in the description are just 
too ambiguous to handle correctly?
Does anyone see a way the highlighted formatting issue can be solved?

Many thanks,

- Tamas




Comment at: clang/lib/Format/TokenAnnotator.cpp:313
+// for example:
+//   void f(volatile ElfW(Addr)* addr = nullptr);
+if (HasStarToken) {

MyDeveloperDay wrote:
> I assume it could be almost anything?
> 
> void f(volatile ElfW(Addr)& addr);
> void f(volatile ElfW(Addr)&& addr);
> void f(volatile ElfW(Addr) const & addr);
> 
> void f(volatile ElfW(Addr,foo)* addr);
> void f(volatile ElfW(Addr,ElfW(Addr) *foo)* addr);
> 
> ? you seem to handle only the * case
Yes, I am handling this case only at the moment.
I am not sure this patch is going to land at all so I spared some work until we 
figure it out.



Comment at: clang/unittests/Format/FormatTest.cpp:7360
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");
+  verifyFormat("const M(x) *foo(M(x) *a = nullptr);");

krasimir wrote:
> This is ambiguous: the `*` could be a binary operator: 
> https://godbolt.org/z/n7Jr-h
This case is ambiguous. Like the case at line 7324, which could also be 
"MACRO()* ptr;"


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

A sample of snippets that this misformats:

- inside other macro:

  EXPECT_EQ(Calc()* bar, baz);

- in member function call:

  BENCHMARK(BM_Foo)->ThreadRange(1, NumCPUs()* 2);

- this causes javascript diffs:

  - const foo = (bar == 'rtl' ? -1 : 1) * (blah || blah || 0);
  + const foo =(bar == 'rtl' ? -1 : 1) * (blah || blah || 0);

- binary operators with casts:

  a = (int)(b()* c);
  rectaa->SetSize((int)(GetWidth(foo)* bar),
  (int)(GetHeight(foo)* bar));
  int f() {
...
  - return static_cast(param) * (a * b + c);
  + return static_cast(param)*(a * b + c);
  }



- some arithmetic expressions:

  float  = 43 * log(sin(a/4) *b);

Test:

  % cat test.cc   
  EXPECT_EQ(Calc() * bar, baz);
  
  BENCHMARK(BM_Foo)->ThreadRange(1, NumCPUs() * 2);
  
  a = (int)(b() * c);
  
  rectaa->SetSize((int)(GetWidth(foo) * bar),
  (int)(GetHeight(foo) * bar));
  
  int f() { return static_cast(param) * (a * b + c); }
  
  float  = 43 * log(sin(a / 4) * b);
  % bin/clang-format -style=google test.cc
  EXPECT_EQ(Calc()* bar, baz);
  
  BENCHMARK(BM_Foo)->ThreadRange(1, NumCPUs()* 2);
  
  a = (int)(b()* c);
  
  rectaa->SetSize((int)(GetWidth(foo)* bar),
  (int)(GetHeight(foo)* bar));
  
  int f() { return static_cast(param)*(a * b + c); }
  
  float  = 43 * log(sin(a / 4)* b);
  % cat test.js
  const foo = (bar == 'rtl' ? -1 : 1) * (blah || blah || 0);
  % bin/clang-format -style=google test.js
  const foo =(bar == 'rtl' ? -1 : 1) * (blah || blah || 0);


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Krasimir Georgiev via Phabricator via cfe-commits]

krasimir added a comment.

I think situations where we infer M(x) to expand to a "type" without 
const/volatile close-by are too ambiguous an we will have the opposite problem: 
formatting certain binary expressions as type pointer/references. Some sort of 
a list of macros that clang-format assumes expand to  a "type" could eliminate 
ambiguities, but has its own problems.
I think the case where there's a const/volatile/typename/typedef etc. around 
M(x) is very interesting; not sure if there are many practical ambiguities 
there.
There was an idea of giving a bunch of #define-s to clang-format (maybe with a 
small subset of what's possible) and use these rules to decide how to format 
matching forms, but that's hard (requires some sort of "virtual expanded token 
sequences" that the formatter should undestrand how to handle) to do and I 
don't know what's the status of that.




Comment at: clang/unittests/Format/FormatTest.cpp:7360
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");
+  verifyFormat("const M(x) *foo(M(x) *a = nullptr);");

This is ambiguous: the `*` could be a binary operator: 
https://godbolt.org/z/n7Jr-h


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:313
+// for example:
+//   void f(volatile ElfW(Addr)* addr = nullptr);
+if (HasStarToken) {

I assume it could be almost anything?

void f(volatile ElfW(Addr)& addr);
void f(volatile ElfW(Addr)&& addr);
void f(volatile ElfW(Addr) const & addr);

void f(volatile ElfW(Addr,foo)* addr);
void f(volatile ElfW(Addr,ElfW(Addr) *foo)* addr);

? you seem to handle only the * case


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Mitchell via Phabricator via cfe-commits]

mitchell-stellar added inline comments.



Comment at: clang/unittests/Format/FormatTest.cpp:7359
   verifyIndependentOfContext("bool a = f() && final.f();");
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");

I would like to see the macro definitions in your tests. It's not clear from 
looking at just the test line that `M` is supposed to be a macro. It looks like 
a syntax error.


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.



Comment at: clang/lib/Format/TokenAnnotator.cpp:164
+  /// Parses CPP qualified function names.
+  bool parse_function_qname(FormatToken *Tok) const {
+while (Tok && Tok->isOneOf(tok::coloncolon, tok::identifier)) {

your naming convention is incorrect it would be `parseFunctionQualifiedName`

what are your trying to do here? are you just trying to skip the to the `(`


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz added a comment.

Friendly ping.


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

https://reviews.llvm.org/D75364



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

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz updated this revision to Diff 247561.

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

https://reviews.llvm.org/D75364

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
@@ -7356,6 +7356,10 @@
   verifyFormat("void f(const MyFinal );");
   verifyIndependentOfContext("bool a = f() && override.f();");
   verifyIndependentOfContext("bool a = f() && final.f();");
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");
+  verifyFormat("const M(x) *foo(M(x) *a = nullptr);");
+  verifyFormat("const M(x) *foo::bar(M(x) *a = nullptr);");
 
   verifyIndependentOfContext("InvalidRegions[*R] = 0;");
 
@@ -7396,6 +7400,10 @@
   verifyGoogleFormat("void f(Bar* a = nullptr, Bar* b);");
   verifyGoogleFormat("template \n"
  "void f(int i = 0, SomeType** temps = NULL);");
+  verifyGoogleFormat("int f(M(x)* p1 = nullptr, const M(x)* p2);");
+  verifyGoogleFormat("M(x)* foo();");
+  verifyGoogleFormat("const M(x)* foo(M(x)* a = nullptr);");
+  verifyGoogleFormat("const M(x)* foo::bar(M(x)* a = nullptr);");
 
   FormatStyle Left = getLLVMStyle();
   Left.PointerAlignment = FormatStyle::PAS_Left;
@@ -7410,6 +7418,11 @@
   verifyFormat("auto x(A&&, B&&, C&&) -> D;", Left);
   verifyFormat("auto x = [](A&&, B&&, C&&) -> D {};", Left);
   verifyFormat("template  X(T&&, T&&, T&&) -> X;", Left);
+  verifyFormat("int f(M(x)* p1 = nullptr, const M(x)* p2, volatile M(x)* p3);",
+   Left);
+  verifyFormat("M(x)* foo();", Left);
+  verifyFormat("const M(x)* foo(M(x)* a = nullptr);", Left);
+  verifyFormat("const M(x)* foo::bar(M(x)* a = nullptr);", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -7537,6 +7550,11 @@
   verifyFormat("A = new SomeType *[Length]();", PointerMiddle);
   verifyFormat("A = new SomeType *[Length];", PointerMiddle);
   verifyFormat("T ** t = new T *;", PointerMiddle);
+  verifyFormat("int f(M(x) * p1 = nullptr, const M(x) * p2 = nullptr);",
+   PointerMiddle);
+  verifyFormat("M(x) * foo();", PointerMiddle);
+  verifyFormat("const M(x) * foo(M(x) * a = nullptr);", PointerMiddle);
+  verifyFormat("const M(x) * foo::bar(M(x) * a = nullptr);", PointerMiddle);
 
   // Member function reference qualifiers aren't binary operators.
   verifyFormat("string // break\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,14 @@
 return false;
   }
 
+  /// Parses CPP qualified function names.
+  bool parse_function_qname(FormatToken *Tok) const {
+while (Tok && Tok->isOneOf(tok::coloncolon, tok::identifier)) {
+  Tok = Tok->Next;
+}
+return Tok && Tok->is(tok::l_paren);
+  }
+
   bool parseParens(bool LookForDecls = false) {
 if (!CurrentToken)
   return false;
@@ -250,6 +258,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+bool HasStarToken = false;
 FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
@@ -299,6 +308,55 @@
   }
 }
 
+// Detect cases where macros are used in function parameter lists,
+// for example:
+//   void f(volatile ElfW(Addr)* addr = nullptr);
+if (HasStarToken) {
+  for (FormatToken *Tok = Left; Tok != CurrentToken; Tok = Tok->Next) {
+// Search for ') *' patterns.
+if (Tok->is(tok::star) && Tok->Previous->is(tok::r_paren)) {
+  // Extend search to left looking for 'X(...) *' patterns.
+  FormatToken *LparenTok = Tok->Previous->MatchingParen;
+  if (LparenTok && LparenTok->is(tok::l_paren) &&
+  LparenTok->Previous) {
+FormatToken *MacroTok = LparenTok->Previous;
+// Decide if 'X' is following "l_paren" of this function,
+// a keyword or a comma.
+if (MacroTok->is(tok::identifier) && MacroTok->Previous &&
+(MacroTok->Previous == Left ||
+ MacroTok->Previous->isOneOf(tok::comma, tok::kw_const,
+ tok::kw_volatile))) {
+  Tok->Type = TT_PointerOrReference;
+  LparenTok->Previous->Type = TT_TypenameMacro;
+}
+  }
+}
+  }
+}
+
+// Detect cases where macros are used in function return types,
+// for example:
+//   const ElfW(Addr)* 

[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz created this revision.
tamas.petz added reviewers: chandlerc, djasper.
tamas.petz added a project: clang-format.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

This patch adds support to handle macros in function parameter list
and function return type.

Consider the following case:

  const ElfW(Addr)* foo(ElfW(Addr)* addr);

This case was handled incorrectly because the star token was
handled as a binary operator. After formatting:

  const ElfW(Addr) * foo(ElfW(Addr) * addr);

This patch adds support for this case.

All format tests pass.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75364

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
@@ -7356,6 +7356,10 @@
   verifyFormat("void f(const MyFinal );");
   verifyIndependentOfContext("bool a = f() && override.f();");
   verifyIndependentOfContext("bool a = f() && final.f();");
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");
+  verifyFormat("const M(x) *foo(M(x) *a = nullptr);");
+  verifyFormat("const M(x) *foo::bar(M(x) *a = nullptr);");
 
   verifyIndependentOfContext("InvalidRegions[*R] = 0;");
 
@@ -7396,6 +7400,10 @@
   verifyGoogleFormat("void f(Bar* a = nullptr, Bar* b);");
   verifyGoogleFormat("template \n"
  "void f(int i = 0, SomeType** temps = NULL);");
+  verifyGoogleFormat("int f(M(x)* p1 = nullptr, const M(x)* p2);");
+  verifyGoogleFormat("M(x)* foo();");
+  verifyGoogleFormat("const M(x)* foo(M(x)* a = nullptr);");
+  verifyGoogleFormat("const M(x)* foo::bar(M(x)* a = nullptr);");
 
   FormatStyle Left = getLLVMStyle();
   Left.PointerAlignment = FormatStyle::PAS_Left;
@@ -7410,6 +7418,11 @@
   verifyFormat("auto x(A&&, B&&, C&&) -> D;", Left);
   verifyFormat("auto x = [](A&&, B&&, C&&) -> D {};", Left);
   verifyFormat("template  X(T&&, T&&, T&&) -> X;", Left);
+  verifyFormat("int f(M(x)* p1 = nullptr, const M(x)* p2, volatile M(x)* p3);",
+Left);
+  verifyFormat("M(x)* foo();", Left);
+  verifyFormat("const M(x)* foo(M(x)* a = nullptr);", Left);
+  verifyFormat("const M(x)* foo::bar(M(x)* a = nullptr);", Left);
 
   verifyIndependentOfContext("a = *(x + y);");
   verifyIndependentOfContext("a = &(x + y);");
@@ -7537,6 +7550,11 @@
   verifyFormat("A = new SomeType *[Length]();", PointerMiddle);
   verifyFormat("A = new SomeType *[Length];", PointerMiddle);
   verifyFormat("T ** t = new T *;", PointerMiddle);
+  verifyFormat("int f(M(x) * p1 = nullptr, const M(x) * p2 = nullptr);",
+   PointerMiddle);
+  verifyFormat("M(x) * foo();", PointerMiddle);
+  verifyFormat("const M(x) * foo(M(x) * a = nullptr);", PointerMiddle);
+  verifyFormat("const M(x) * foo::bar(M(x) * a = nullptr);", PointerMiddle);
 
   // Member function reference qualifiers aren't binary operators.
   verifyFormat("string // break\n"
Index: clang/lib/Format/TokenAnnotator.cpp
===
--- clang/lib/Format/TokenAnnotator.cpp
+++ clang/lib/Format/TokenAnnotator.cpp
@@ -160,6 +160,14 @@
 return false;
   }
 
+  /// Parses CPP qualified function names.
+  bool parse_function_qname(FormatToken *Tok) const {
+while (Tok && Tok->isOneOf(tok::coloncolon, tok::identifier)) {
+  Tok = Tok->Next;
+}
+return Tok && Tok->is(tok::l_paren);
+  }
+
   bool parseParens(bool LookForDecls = false) {
 if (!CurrentToken)
   return false;
@@ -250,6 +258,7 @@
 bool HasMultipleParametersOnALine = false;
 bool MightBeObjCForRangeLoop =
 Left->Previous && Left->Previous->is(tok::kw_for);
+bool HasStarToken = false;
 FormatToken *PossibleObjCForInToken = nullptr;
 while (CurrentToken) {
   // LookForDecls is set when "if (" has been seen. Check for
@@ -299,6 +308,55 @@
   }
 }
 
+// Detect cases where macros are used in function parameter lists,
+// for example:
+//   void f(volatile ElfW(Addr)* addr = nullptr);
+if (HasStarToken) {
+  for (FormatToken *Tok = Left; Tok != CurrentToken; Tok = Tok->Next) {
+// Search for ') *' patterns.
+if (Tok->is(tok::star) && Tok->Previous->is(tok::r_paren)) {
+  // Extend search to left looking for 'X(...) *' patterns.
+  FormatToken *LparenTok = Tok->Previous->MatchingParen;
+  if (LparenTok && LparenTok->is(tok::l_paren) &&
+  LparenTok->Previous) {
+FormatToken *MacroTok = LparenTok->Previous;
+// Decide if 'X' is following "l_paren" of this function,
+// a keyword or a comma.
+if (MacroTok->is(tok::identifier) && MacroTok->Previous &&
+