[clang-format] [PR39719] clang-format converting object-like macro to function-like macro

[MyDeveloperDay]

Summary

Clang format is incorrectly formatting macros where keywords are redefined

See https://bugs.llvm.org/show_bug.cgi?id=39719

The following code

- #define true ((foo)1)
- #define false ((foo)0)

becomes

+ #define true((foo)1)
+ #define false((foo)0)

[owenpan]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

I think it can be more precise and simplified to something like this:

if (Left.Previous && Left.Previous->is(tok::pp_define) &&
    Left.isNot(tok::identifier) && Right.is(tok::l_paren))

[klimek]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

Why don't we have the same problem for identifier? Is that already solved and the problem is that this is a keyword redefinition?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

Yes the identifier seems to work ok, but when its a keyword redfinition the identifier is replaced with the token for the keyword i.e. tok::kw_true or tok::kw_false

[klimek]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

And the idea is that for non-ID
#define true(x) x
won't work anyway? (otherwise this patch would be incorrect, right?)

Have you looked at where we detect the diff between
#define a(x) x
and
#define a (x)
in the identifier case and looked we could add common keyword macro cases there?

[MyDeveloperDay]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

I see what you mean, this path will reformat the false #define incorrectly

#define true ((foo)1)
#define false(x) x

will be transformed to

#define true ((foo)1)
#define false (x) x

[klimek]

clang/lib/Format/TokenAnnotator.cpp

2467–2470

Exactly.

[MyDeveloperDay]

@klimek one possible solution to this might be to replace the "keyword" back to an identifier in a '#define <keywoord>' scenario

Maybe something like this?

bool FormatTokenLexer::tryConvertKeyWordDefines() {
  // ensure #define keyword x = tok::hash,tok::identifier,tok::identifier
  if (Tokens.size() < 3)
    return false;

  auto &Hash = *(Tokens.end() - 3);
  auto &Define = *(Tokens.end() - 2);
  auto &Keyword = *(Tokens.end() - 1);

  if (!Hash->is(tok::hash))
    return false;

  if (!Define->is(tok::identifier))
    return false;

  // Already an identifier
  if (Keyword->is(tok::identifier))
    return false;

  if (!Define->Tok.getIdentifierInfo() ||
      Define->Tok.getIdentifierInfo()->getPPKeywordID() != tok::pp_define)
    return false;

  // switch the type to be an identifier
  Keyword->Tok.setKind(tok::identifier);
  return true;
}

[owenpan]

A more straightforward way, IMO, is to add to the spaceRequiredBetween function a separate if statement that returns false for the sequence of tokens: #, define, tok::identifier, and (

[owenpan]

Actually, there is a neater way: https://reviews.llvm.org/D60853

[MyDeveloperDay]

Abandoning in favor of D60853: clang-format converts a keyword macro definition to a macro function

clang/lib/Format/TokenAnnotator.cpp

bool TokenAnnotator::spaceRequiredBeforeParens(const FormatToken &Right) const {
   return Style.SpaceBeforeParens == FormatStyle::SBPO_Always ||
          (Style.SpaceBeforeParens == FormatStyle::SBPO_NonEmptyParentheses &&
           Right.ParameterCount > 0);
 }
 
 bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
                                           const FormatToken &Left,
                                           const FormatToken &Right) {
+  // if in a #define and a keyword is being defined e.g. #define true (1)
+  // ensure the space between the keyword and '(' is preserved
+  if (Line.InPPDirective && Right.is(tok::l_paren) &&
+      !Left.is(tok::identifier) && Left.Previous &&
+      Left.Previous->is(tok::identifier) && Left.Previous->Previous &&
+      Left.Previous->Previous->is(tok::hash))
+    return true;
   if (Left.is(tok::kw_return) && Right.isNot(tok::semi))
     return true;
   if (Left.is(Keywords.kw_assert) && Style.Language == FormatStyle::LK_Java)
     return true;
   if (Style.ObjCSpaceAfterProperty && Line.Type == LT_ObjCProperty &&
       Left.Tok.getObjCKeywordID() == tok::objc_property)
     return true;
   if (Right.is(tok::hashhash))

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, GuessLanguageWithChildLines) {
   EXPECT_EQ(
       FormatStyle::LK_Cpp,
       guessLanguage("foo.h", "#define FOO ({ foo(); ({ std::string s; }) })"));
   EXPECT_EQ(
       FormatStyle::LK_ObjC,
       guessLanguage("foo.h", "#define FOO ({ foo(); ({ NSString *s; }) })"));
 }
 
+TEST_F(FormatTest, MacroKeyWordsAndParents) {
+
+  format::FormatStyle Style = format::getLLVMStyle();
+  verifyFormat("#define TRUE ((foo)1)", Style);
+  verifyFormat("#define throw ((foo)1)", Style);
+  verifyFormat("#define true ((foo)1)", Style);
+  verifyFormat("#define false ((foo)1)", Style);
+  verifyFormat("#define sizeof ((foo)1)", Style);
+  verifyFormat("#define new ((foo)1)", Style);
+  verifyFormat("#define delete ((foo)1)", Style);
+  verifyFormat("#define for ((foo)1)", Style);
+  verifyFormat("#define override ((foo)1)", Style);
+  verifyFormat("#define else ((foo)1)", Style);
+  verifyFormat("#define true 1", Style);
+  verifyFormat("#define true foo", Style);
+  verifyFormat("#define true foo()", Style);
+  verifyFormat("#define NO_LENGTH (~(uint64_t)0)", Style);
+}
+
 } // end namespace
 } // end namespace format
 } // end namespace clang