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

[tamas.petz]

Summary

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.

[tamas.petz]

Friendly ping.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

164

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 (

[mitchell-stellar]

clang/unittests/Format/FormatTest.cpp

7359

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.

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

313

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

[krasimir]

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.

clang/unittests/Format/FormatTest.cpp

7360

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

[krasimir]

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);
rectaaaaaaaaaaaaaa->SetSize((int)(GetWidth(foo)* bar),
                            (int)(GetHeight(foo)* bar));
int f() {
  ...
- return static_cast<int>(param) * (a * b + c);
+ return static_cast<int>(param)*(a * b + c);
}

• some arithmetic expressions:

float aaaa = 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);

rectaaaaaaaaaaaaaa->SetSize((int)(GetWidth(foo) * bar),
                            (int)(GetHeight(foo) * bar));

int f() { return static_cast<int>(param) * (a * b + c); }

float aaaa = 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);

rectaaaaaaaaaaaaaa->SetSize((int)(GetWidth(foo)* bar),
                            (int)(GetHeight(foo)* bar));

int f() { return static_cast<int>(param)*(a * b + c); }

float aaaa = 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);

[tamas.petz]

@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

clang/lib/Format/TokenAnnotator.cpp

313

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.

clang/unittests/Format/FormatTest.cpp

7360

This case is ambiguous. Like the case at line 7324, which could also be "MACRO()* ptr;"

[MyDeveloperDay]

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);

[tamas.petz]

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

Looks like this change should be abandoned.

[MyDeveloperDay]

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.

[tamas.petz]

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.

clang/lib/Format/TokenAnnotator.cpp

while (CurrentToken) {
         }
       }
       if (!consumeToken())
         return false;
     }
     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;
     FormatToken *Left = CurrentToken->Previous;
     Left->ParentBracket = Contexts.back().ContextKind;
     ScopedContextCreator ContextCreator(*this, tok::l_paren, 1);
 
     // FIXME: This is a bit of a hack. Do better.
bool parseParens(bool LookForDecls = false) {
     // void (^ObjCBlock)(void);
     bool MightBeFunctionType = !Contexts[Contexts.size() - 2].IsExpression;
     bool ProbablyFunctionType =
         CurrentToken->isOneOf(tok::star, tok::amp, tok::caret);
     bool HasMultipleLines = false;
     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
       // 'identifier' '*' 'identifier' followed by not '=' -- this
       // '*' has to be a binary operator but determineStarAmpUsage() will
       // categorize it as an unary operator, so set the right type here.
       if (LookForDecls && CurrentToken->Next) {
         FormatToken *Prev = CurrentToken->getPreviousNonComment();
while (CurrentToken) {
           //   auto my_lambda = MARCO((Type *type, int i) { .. body .. });
           for (FormatToken *Tok = Left; Tok != CurrentToken; Tok = Tok->Next) {
             if (Tok->is(TT_BinaryOperator) &&
                 Tok->isOneOf(tok::star, tok::amp, tok::ampamp))
               Tok->Type = TT_PointerOrReference;
           }
         }
 
+        // 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)* f();
+        if (CurrentToken->Next && CurrentToken->Next->is(tok::star)) {
+          if (parse_function_qname(CurrentToken->Next->Next)) {
+            bool Found = true;
+            FormatToken *const Prev = CurrentToken->MatchingParen->Previous;
+            if (Prev && Prev->is(tok::identifier)) {
+              for (FormatToken *Tok = Prev; Tok; Tok = Tok->Previous) {
+                if (!Tok->is(TT_Unknown)) {
+                  Found = false;
+                  break;
+                } else if (Prev->is(tok::semi))
+                  break;
+              }
+            }
+            if (Found) {
+              CurrentToken->Next->Type = TT_PointerOrReference;
+              Prev->Type = TT_TypenameMacro;
+            }
+          }
+        }
+
         if (StartsObjCMethodExpr) {
           CurrentToken->Type = TT_ObjCMethodExpr;
           if (Contexts.back().FirstObjCSelectorName) {
             Contexts.back().FirstObjCSelectorName->LongestObjCSelectorName =
                 Contexts.back().LongestObjCSelectorName;
           }
         }
 
while (CurrentToken) {
         PossibleObjCForInToken = CurrentToken;
         PossibleObjCForInToken->Type = TT_ObjCForIn;
       }
       // When we discover a 'new', we set CanBeExpression to 'false' in order to
       // parse the type correctly. Reset that after a comma.
       if (CurrentToken->is(tok::comma))
         Contexts.back().CanBeExpression = true;
 
+      if (CurrentToken->is(tok::star))
+        HasStarToken = true;
+
       FormatToken *Tok = CurrentToken;
       if (!consumeToken())
         return false;
       updateParameterCount(Left, Tok);
       if (CurrentToken && CurrentToken->HasUnescapedNewline)
         HasMultipleLines = true;
     }
     return false;

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyFormat("Constructor() : a(a), area(a, width * height) {}");
   verifyGoogleFormat("MACRO Constructor(const int& i) : a(a), b(b) {}");
   verifyFormat("void f() { f(a, c * d); }");
   verifyFormat("void f() { f(new a(), c * d); }");
   verifyFormat("void f(const MyOverride &override);");
   verifyFormat("void f(const MyFinal &final);");
   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;");
 
   verifyIndependentOfContext("A<int *> a;");
   verifyIndependentOfContext("A<int **> a;");
   verifyIndependentOfContext("A<int *, int *> a;");
   verifyIndependentOfContext("A<int *[]> a;");
   verifyIndependentOfContext(
TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyGoogleFormat("Type* t = const_cast<T*>(&*x);");
   verifyGoogleFormat("Type* t = x++ * y;");
   verifyGoogleFormat(
       "const char* const p = reinterpret_cast<const char* const>(q);");
   verifyGoogleFormat("void f(int i = 0, SomeType** temps = NULL);");
   verifyGoogleFormat("void f(Bar* a = nullptr, Bar* b);");
   verifyGoogleFormat("template <typename T>\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;
   verifyFormat("x = *a(x) = *a(y);", Left);
   verifyFormat("for (;; *a = b) {\n}", Left);
   verifyFormat("return *this += 1;", Left);
   verifyFormat("throw *x;", Left);
   verifyFormat("delete *x;", Left);
   verifyFormat("typedef typeof(int(int, int))* MyFuncPtr;", Left);
   verifyFormat("[](const decltype(*a)* ptr) {}", Left);
   verifyFormat("typedef typeof /*comment*/ (int(int, int))* MyFuncPtr;", Left);
   verifyFormat("auto x(A&&, B&&, C&&) -> D;", Left);
   verifyFormat("auto x = [](A&&, B&&, C&&) -> D {};", Left);
   verifyFormat("template <class T> X(T&&, T&&, T&&) -> X<T>;", 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);");
   verifyIndependentOfContext("*(x + y).call();");
   verifyIndependentOfContext("&(x + y)->call();");
   verifyFormat("void f() { &(*I).first; }");
 
   verifyIndependentOfContext("f(b * /* confusing comment */ ++c);");
TEST_F(FormatTest, UnderstandsUsesOfStarAndAmp) {
   verifyFormat("Test::Test(int b) : a(b * b) {}", PointerMiddle);
   verifyFormat("A<int *> a;", PointerMiddle);
   verifyFormat("A<int **> a;", PointerMiddle);
   verifyFormat("A<int *, int *> a;", PointerMiddle);
   verifyFormat("A<int *[]> a;", PointerMiddle);
   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"
                "operator()() & {}");
   verifyFormat("string // break\n"
                "operator()() && {}");
   verifyGoogleFormat("template <typename T>\n"
                      "auto x() & -> int {}");