[clang-format] Support custom If macros

[• vlovich]

Summary

Like with ForEachMacros, support user to provide additional macros that act like if statements.

This makes it possible to support KJ_IF_MAYBE from the cap'n'proto library:
https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#maybes.

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

[• vlovich]

clang/include/clang/Format/Format.h

2098

Should I put a hyperlink to https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#maybes?

[HazardyKnusperkeks]

Looks good, but please add a test with the else IF and just else case.

Also for me it would be ok to add your KJ_IF_MAYBE to the default configuration. But I have no experience on that.

clang/include/clang/Format/Format.h

2098

Maybe. *scnr*

[• vlovich]

Review response:

• Added additional tests.
• Added KJ_IF_MAYBE to default IfMacros.
• Added links to KJ documentation.

[• vlovich]

clang/include/clang/Format/Format.h

2098

Well done. Took me a second.

[• vlovich]

Updated release notes.

[• vlovich]

Fixed missing _ after the hyperlink to the KJ link in the Style options documentation. I'm assuming that's required formatting for hyperlinks by what renders the markdown.

[MyDeveloperDay]

clang/docs/ReleaseNotes.rst

252 ↗

(On Diff #346870)

stiled? did you mean styled?

clang/lib/Format/FormatTokenLexer.cpp

1019

Is there any chance you could leave a comment here as to why this is needed, I can't work it out?

clang/lib/Format/TokenAnnotator.cpp

3025

I'll let you decide if you think we need another SBPO_XXX style?

clang/unittests/Format/FormatTest.cpp

19699

this should support `MYIF<space>(` by processing SpacesInConditionalStatement as we do with FOREACH

[• vlovich]

Review response. Updated the options I changed via the dump_format_style.py script but didn't ingest all the other changes it generated.
Changed the SBPO option to have a more generic name that also applies to IF macros.
Fix typo in release notes.

clang/docs/ReleaseNotes.rst

252 ↗

(On Diff #346870)

Whoops. Yes I did.

clang/lib/Format/FormatTokenLexer.cpp

1019

Will do my best. TLDR: The token is otherwise "unknown" & the conditional formatting logic depends on the lexer token value. This avoids having to rewrite all the existing code & seemed to make sense since we want this token treated like an if.

I don't of course know if this is The Best Way (tm). It's just the minimal change I figured out would get the formatting to work properly but happy to apply feedback from someone more intimately familiar with the internals of the formatter as I'm a complete novice here.

clang/lib/Format/TokenAnnotator.cpp

3025

I thought about it but I wasn't was really sure how to add it in a way that would make sense. Do you think people would want to apply consistent SBPO styling for IF & FOREACH macros or want fine-grained control? If the former, then I can just check the foreach macro & maybe rename it to SBPO_ControlStatementsExceptMacros (maintaining the old name for back compat). If the latter, then it would seem like we need a separate boolean that controls whether SBPO_ControlStatements would apply?

My gut is probably the "maintain consistency" option is fine for now so I've gone ahead & applied that change in the latest diff.

[HazardyKnusperkeks]

clang/include/clang/Format/Format.h

2980

Why did you change this?

[MyDeveloperDay]

This kind of looks ok to me, I'm not sure about the "lexer" part, I guess I don't know the impact but I'd expect it to be small because it's conditional on your IfMacro

I'd like @curdeius or @krasimir to take a look.

[• vlovich]

clang/include/clang/Format/Format.h

2980

Per the discussion below.

MyDeveloperDay
I'll let you decide if you think we need another SBPO_XXX style?

Me
I thought about it but I wasn't was really sure how to add it in a way that would make sense. Do you think people would want to apply consistent SBPO styling for IF & FOREACH macros or want fine-grained control? If the former, then I can just check the foreach macro & maybe rename it to SBPO_ControlStatementsExceptMacros (maintaining the old name for back compat). If the latter, then it would seem like we need a separate boolean that controls whether SBPO_ControlStatements would apply?
My gut is probably the "maintain consistency" option is fine for now so I've gone ahead & applied that change in the latest diff.

This felt like a simpler solution because otherwise you would either end up with SBPO_ControlStatementsForEachMacros, SBPO_ControlStatementsExceptIfAndForEachMacros, SBPO_ControlStatementsExceptIfMacros which just feels extremely confusing (& for now I'm assuming you'll want a similar style for ForEach & If macros). Arguably at the point where you want distinct SBPO styling of these control-like macros, you would be moved them out into a separate option since it's really orthogonal to the other settings.

Open to suggestions of course.

[curdeius]

Thanks for working on this!
Looks pretty nice, but please clang-format the changes and add some more test cases (cf. inline comment).

Concerning the lexer part, I wouldn't do anything cleaner.

clang/unittests/Format/FormatTest.cpp

19713

Please add a test to see the interaction with AllowShortIfStatementsOnASingleLine, and maybe BreakBeforeBraces, BraceWrappingAfterControlStatementStyle, SpacesInConditionalStatement.

[• vlovich]

Just back from vacation so will try to polish this off today.

[• vlovich]

Review response

I've updated coverage for AllowShortIfStatementsOnASingleLine and
BreakBeforeBraces. SpacesInConditionalStatement is what the existing
tests were covering so I don't think there's anything there to update.
I looked & couldn't find any tests for
BraceWrappingAfterControlStatementStyle so my (admittedly lazy)
preference is to skip that for now if possible.

[• vlovich]

Fix clang-format issue

[HazardyKnusperkeks]

Seems to be okay for me.

[• vlovich]

Rebase onto main.

[MyDeveloperDay]

LGTM

clang/docs/ClangFormatStyleOptions.rst

**ForEachMacros** (``std::vector<std::string>``)
   In the .clang-format configuration file, this can be configured like:
 
   .. code-block:: yaml
 
     ForEachMacros: ['RANGES_FOR', 'FOREACH']
 
   For example: BOOST_FOREACH.
 
+**IfMacros** (``std::vector<std::string>``)
+  A vector of macros that should be interpreted as conditionals
+  instead of as function calls.
+
+  These are expected to be macros of the form:
+
+  .. code-block:: c++
+
+    IF(...)
+      <conditional-body>
+    else IF(...)
+      <conditional-body>
+
+  In the .clang-format configuration file, this can be configured like:
+
+  .. code-block:: yaml
+
+    IfMacros: ['IF']
+
+  For example: KJ_IF_MAYBE.
+
 **IncludeBlocks** (``IncludeBlocksStyle``)
   Dependent on the value, multiple ``#include`` blocks can be sorted
   as one and divided based on category.
 
   Possible values:
 
   * ``IBS_Preserve`` (in configuration: ``Preserve``)
     Sort each ``#include`` block separately.

clang/include/clang/Format/Format.h

   /// In the .clang-format configuration file, this can be configured like:
   /// \code{.yaml}
   ///   ForEachMacros: ['RANGES_FOR', 'FOREACH']
   /// \endcode
   ///
   /// For example: BOOST_FOREACH.
   std::vector<std::string> ForEachMacros;
 
+  /// A vector of macros that should be interpreted as conditionals
+  /// instead of as function calls.
+  ///
+  /// These are expected to be macros of the form:
+  /// \code
+  ///   IF(...) {
+  ///     <if body>
+  ///   } else {
+  ///   }
+  /// \endcode
+  ///
+  /// In the .clang-format configuration file, this can be configured like:
+  /// \code{.yaml}
+  ///   IfMacros: ['IF']
+  /// \endcode
+  ///
+  /// For example: KJ_IF_MAYBE.
+  std::vector<std::string> IfMacros;
+
   /// \brief A vector of macros that should be interpreted as type declarations
   /// instead of as function calls.
   ///
   /// These are expected to be macros of the form:
   /// \code
   ///   STACK_OF(...)
   /// \endcode
   ///
enum SpaceBeforeParensOptions : unsigned char {
     /// treated as function calls instead of control statements.
     /// \code
     ///    void f() {
     ///      Q_FOREACH(...) {
     ///        f();
     ///      }
     ///    }
     /// \endcode
     SBPO_ControlStatementsExceptForEachMacros,
     /// Put a space before opening parentheses only if the parentheses are not
     /// empty i.e. '()'
     /// \code
     ///   void() {
     ///     if (true) {
     ///       f();
     ///       g (x, y, z);
     ///     }

clang/lib/Format/Format.cpp

static void mapping(IO &IO, FormatStyle &Style) {
     IO.mapOptional("EmptyLineAfterAccessModifier",
                    Style.EmptyLineAfterAccessModifier);
     IO.mapOptional("EmptyLineBeforeAccessModifier",
                    Style.EmptyLineBeforeAccessModifier);
     IO.mapOptional("ExperimentalAutoDetectBinPacking",
                    Style.ExperimentalAutoDetectBinPacking);
     IO.mapOptional("FixNamespaceComments", Style.FixNamespaceComments);
     IO.mapOptional("ForEachMacros", Style.ForEachMacros);
+    IO.mapOptional("IfMacros", Style.IfMacros);
     IO.mapOptional("StatementAttributeLikeMacros",
                    Style.StatementAttributeLikeMacros);
     IO.mapOptional("IncludeBlocks", Style.IncludeStyle.IncludeBlocks);
     IO.mapOptional("IncludeCategories", Style.IncludeStyle.IncludeCategories);
     IO.mapOptional("IncludeIsMainRegex", Style.IncludeStyle.IncludeIsMainRegex);
     IO.mapOptional("IncludeIsMainSourceRegex",
                    Style.IncludeStyle.IncludeIsMainSourceRegex);
     IO.mapOptional("IndentAccessModifiers", Style.IndentAccessModifiers);

clang/lib/Format/FormatToken.h

#define LIST_TOKEN_TYPES                                                       \
   TYPE(DesignatedInitializerPeriod)                                            \
   TYPE(DictLiteral)                                                            \
   TYPE(FatArrow)                                                               \
   TYPE(ForEachMacro)                                                           \
   TYPE(FunctionAnnotationRParen)                                               \
   TYPE(FunctionDeclarationName)                                                \
   TYPE(FunctionLBrace)                                                         \
   TYPE(FunctionTypeLParen)                                                     \
+  TYPE(IfMacro)                                                                \
   TYPE(ImplicitStringLiteral)                                                  \
   TYPE(InheritanceColon)                                                       \
   TYPE(InheritanceComma)                                                       \
   TYPE(InlineASMBrace)                                                         \
   TYPE(InlineASMColon)                                                         \
   TYPE(InlineASMSymbolicNameLSquare)                                           \
   TYPE(JavaAnnotation)                                                         \
   TYPE(JsComputedPropertyName)                                                 \

clang/lib/Format/FormatTokenLexer.cpp

: FormatTok(nullptr), IsFirstToken(true), StateStack({LexerState::NORMAL}),
       FormattingDisabled(false), MacroBlockBeginRegex(Style.MacroBlockBegin),
       MacroBlockEndRegex(Style.MacroBlockEnd) {
   Lex.reset(new Lexer(ID, SourceMgr.getBufferOrFake(ID), SourceMgr,
                       getFormattingLangOpts(Style)));
   Lex->SetKeepWhitespaceMode(true);
 
   for (const std::string &ForEachMacro : Style.ForEachMacros)
     Macros.insert({&IdentTable.get(ForEachMacro), TT_ForEachMacro});
+  for (const std::string &IfMacro : Style.IfMacros)
+    Macros.insert({&IdentTable.get(IfMacro), TT_IfMacro});
   for (const std::string &AttributeMacro : Style.AttributeMacros)
     Macros.insert({&IdentTable.get(AttributeMacro), TT_AttributeMacro});
   for (const std::string &StatementMacro : Style.StatementMacros)
     Macros.insert({&IdentTable.get(StatementMacro), TT_StatementMacro});
   for (const std::string &TypenameMacro : Style.TypenameMacros)
     Macros.insert({&IdentTable.get(TypenameMacro), TT_TypenameMacro});
   for (const std::string &NamespaceMacro : Style.NamespaceMacros)
     Macros.insert({&IdentTable.get(NamespaceMacro), TT_NamespaceMacro});
FormatToken *FormatTokenLexer::getNextToken() {
 
   if (Style.isCpp()) {
     auto it = Macros.find(FormatTok->Tok.getIdentifierInfo());
     if (!(Tokens.size() > 0 && Tokens.back()->Tok.getIdentifierInfo() &&
           Tokens.back()->Tok.getIdentifierInfo()->getPPKeywordID() ==
               tok::pp_define) &&
         it != Macros.end()) {
       FormatTok->setType(it->second);
+      if (it->second == TT_IfMacro) {
+        FormatTok->Tok.setKind(tok::kw_if);
+      }
     } else if (FormatTok->is(tok::identifier)) {
       if (MacroBlockBeginRegex.match(Text)) {
         FormatTok->setType(TT_MacroBlockBegin);
       } else if (MacroBlockEndRegex.match(Text)) {
         FormatTok->setType(TT_MacroBlockEnd);
       }
     }
   }

clang/lib/Format/TokenAnnotator.cpp

private:
 
   void resetTokenMetadata(FormatToken *Token) {
     if (!Token)
       return;
 
     // Reset token type in case we have already looked at it and then
     // recovered from an error (e.g. failure to find the matching >).
     if (!CurrentToken->isOneOf(
-            TT_LambdaLSquare, TT_LambdaLBrace, TT_AttributeMacro,
+            TT_LambdaLSquare, TT_LambdaLBrace, TT_AttributeMacro, TT_IfMacro,
             TT_ForEachMacro, TT_TypenameMacro, TT_FunctionLBrace,
             TT_ImplicitStringLiteral, TT_InlineASMBrace, TT_FatArrow,
             TT_LambdaArrow, TT_NamespaceMacro, TT_OverloadedOperator,
             TT_RegexLiteral, TT_TemplateString, TT_ObjCStringLiteral,
             TT_UntouchableMacroFunc, TT_ConstraintJunctions,
             TT_StatementAttributeLikeMacro))
       CurrentToken->setType(TT_Unknown);
     CurrentToken->Role.reset();
bool TokenAnnotator::spaceRequiredBetween(const AnnotatedLine &Line,
   if (Right.is(tok::l_paren)) {
     if ((Left.is(tok::r_paren) && Left.is(TT_AttributeParen)) ||
         (Left.is(tok::r_square) && Left.is(TT_AttributeSquare)))
       return true;
     if (Style.SpaceBeforeParens ==
             FormatStyle::SBPO_ControlStatementsExceptForEachMacros &&
         Left.is(TT_ForEachMacro))
       return false;
+    if (Left.is(TT_IfMacro)) {
+      return false;
+    }
     return Line.Type == LT_ObjCDecl || Left.is(tok::semi) ||
            (Style.SpaceBeforeParens != FormatStyle::SBPO_Never &&
             (Left.isOneOf(tok::pp_elif, tok::kw_for, tok::kw_while,
                           tok::kw_switch, tok::kw_case, TT_ForEachMacro,
                           TT_ObjCForIn) ||
              Left.isIf(Line.Type != LT_PreprocessorDirective) ||
              (Left.isOneOf(tok::kw_try, Keywords.kw___except, tok::kw_catch,
                            tok::kw_new, tok::kw_delete) &&

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, ParsesConfiguration) {
   BoostForeach.push_back("BOOST_FOREACH");
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH]", ForEachMacros, BoostForeach);
   std::vector<std::string> BoostAndQForeach;
   BoostAndQForeach.push_back("BOOST_FOREACH");
   BoostAndQForeach.push_back("Q_FOREACH");
   CHECK_PARSE("ForEachMacros: [BOOST_FOREACH, Q_FOREACH]", ForEachMacros,
               BoostAndQForeach);
 
+  Style.IfMacros.clear();
+  std::vector<std::string> CustomIfs;
+  CustomIfs.push_back("MYIF");
+  CHECK_PARSE("IfMacros: [MYIF]", IfMacros, CustomIfs);
+
   Style.AttributeMacros.clear();
   CHECK_PARSE("BasedOnStyle: LLVM", AttributeMacros,
               std::vector<std::string>{"__capability"});
   CHECK_PARSE("AttributeMacros: [attr1, attr2]", AttributeMacros,
               std::vector<std::string>({"attr1", "attr2"}));
 
   Style.StatementAttributeLikeMacros.clear();
   CHECK_PARSE("StatementAttributeLikeMacros: [emit,Q_EMIT]",
TEST_F(FormatTest, AmbersandInLamda) {
   AlignStyle.PointerAlignment = FormatStyle::PAS_Left;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
   AlignStyle.PointerAlignment = FormatStyle::PAS_Right;
   verifyFormat("auto lambda = [&a = a]() { a = 2; };", AlignStyle);
 }
 
 TEST_F(FormatTest, SpacesInConditionalStatement) {
   FormatStyle Spaces = getLLVMStyle();
+  Spaces.IfMacros.clear();
+  Spaces.IfMacros.push_back("MYIF");
   Spaces.SpacesInConditionalStatement = true;
   verifyFormat("for ( int i = 0; i; i++ )\n  continue;", Spaces);
   verifyFormat("if ( !a )\n  return;", Spaces);
   verifyFormat("if ( a )\n  return;", Spaces);
   verifyFormat("if constexpr ( a )\n  return;", Spaces);
+  verifyFormat("MYIF( a )\n  return;", Spaces);
   verifyFormat("switch ( a )\ncase 1:\n  return;", Spaces);
   verifyFormat("while ( a )\n  return;", Spaces);
   verifyFormat("while ( (a && b) )\n  return;", Spaces);
   verifyFormat("do {\n} while ( 1 != 0 );", Spaces);
   verifyFormat("try {\n} catch ( const std::exception & ) {\n}", Spaces);
   // Check that space on the left of "::" is inserted as expected at beginning
   // of condition.
   verifyFormat("while ( ::func() )\n  return;", Spaces);
 }
 
 TEST_F(FormatTest, AlternativeOperators) {
   // Test case for ensuring alternate operators are not
   // combined with their right most neighbour.
   verifyFormat("int a and b;");
   verifyFormat("int a and_eq b;");
   verifyFormat("int a bitand b;");
   verifyFormat("int a bitor b;");
   verifyFormat("int a compl b;");
   verifyFormat("int a not b;");
   verifyFormat("int a not_eq b;");
   verifyFormat("int a or b;");
   verifyFormat("int a xor b;");