[clang-format] Handle statement macro expressions in switch block

[HazardyKnusperkeks]

Summary

Cases in switch block can be defined as macros. Now clang-format binpacks
them in one line. This patch make statement macro cases behavior as usual
'case' in switch block.
For example:

#define PROGRESS(x)              \
    ...                          \
    case PROG_##x

Formatting now:
switch (x)
{
case 0:
    ...
    PROGRESS(var):
    ...
}

With patch:
switch (x)
{
case 0:
    ...
PROGRESS(var):
    ...
}

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

1054

Aren't you saying here every statement macro is a case statement? I don't think that is going to work everywhere?

[MyDeveloperDay]

#clang-format is a project not a groups of reviewers

To determine who the reviewers are, its best to look at the "git log" in clang-format areas and see who is making the changes, or look at the OWNERSHIP files

I'm happy to review clang-format items when I can.

[MyDeveloperDay]

I'm not completely convinced this is going to work, but I'm happy to be persuaded incase I misunderstood

[HazardyKnusperkeks]

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

[anastasiia_lukianenko]

In D92168#2690211, @HazardyKnusperkeks wrote:

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

The patch was originally made as a fix for unexpected behavior. I'm not sure that someone would use a variant where the macro is not recognized as a case block... That's why I don't think the new flag is really needed.

[HazardyKnusperkeks]

In D92168#2693972, @anastasiia_lukianenko wrote:

In D92168#2690211, @HazardyKnusperkeks wrote:

This looks okay, I think. But it should definitely add a release note and extend the documentation on statement macros.
Now I'm thinking maybe it should add a new flag to activate that behavior?

The patch was originally made as a fix for unexpected behavior. I'm not sure that someone would use a variant where the macro is not recognized as a case block... That's why I don't think the new flag is really needed.

I said maybe, I don't have a strong opinion on that one, for the documentation on the other hand I have one.

clang/unittests/Format/FormatTest.cpp

3502

Is this needed?

[anastasiia_lukianenko]

clang/unittests/Format/FormatTest.cpp

3502

Yes, because this type of colon is recognized as CtorInitializerColon. Without SpaceBeforeCtorInitializerColon = false the result will be:

"FOO(var) :"

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2925

I just feel this constraint is too lose its say all macrostatements which have a trailing colon are a label?

MACRO(x) :

its hard to think of a non "trite" example that counters that but

CLASS(x) : public Foo
{
};

might be one such situation if user have added CLASS as a statement macro (which may/maynot be the correct thing to do)

to me I feel this needs something like the following to help disambiguate it? (thoughts?)

parseStatementMacro(bool inSwitchStatement){
  ...
if (inSwitchStatement && FormatTok->is(tok::colon))
    parseLabel();
 ...
}

clang/unittests/Format/FormatTest.cpp

3503

is there a reason you are not using verifyFormat()

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

3552 ↗

(On Diff #338960)

This has to be done in Format.h and then updated by the DumpFormatStyles.py script.

clang/lib/Format/UnwrappedLineParser.cpp

2925

I was actually thinking if there is a case where : isn't a label, but yeah that seems to be such a case. That should at least be covered by the tests.

clang/unittests/Format/FormatTest.cpp

3502

Is there a test to cover plain LLVM style? If not please add one.

[anastasiia_lukianenko]

clang/unittests/Format/FormatTest.cpp

3503

No, I can change if it's better to use verifyFormat()

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

3503

If input and result are the same it should almost always be verifyFormat. It's easier to read and it ensures that the formatting is stable, because it removes all whitespace.

[HazardyKnusperkeks]

clang/docs/ClangFormatStyleOptions.rst

3552 ↗

(On Diff #338960)

It now is in Format.h, but the script did not run? :)

[curdeius]

clang/lib/Format/UnwrappedLineParser.cpp

1044

Nit: please use CamelCase for parameters and local variables.

1209

2920

clang/unittests/Format/FormatTest.cpp

3526

Please add a test with a class inheritance inside switch.
Something including:

switch (x) {
class S1 {};
class S2 : S1 {};
case ...
    class S3 {};
    class S4 : public S3 {};
};

[MyDeveloperDay]

clang/include/clang/Format/Format.h

2033

if you change Format.h you have to run docs/tools/dump_format.py this will regenerate the doc/ClangFormatStyleOptions.rst file which need to be part of any review that touches Format.h

[anastasiia_lukianenko]

clang/unittests/Format/FormatTest.cpp

3526

I want to be sure I understand you the right way before pushing changes.
Now the clang-format for your code gives the following formatting:

switch (x) {
  class S1 {};
  class S2 : S1 {};
case 0:
  class S3 {};
  class S4 : public S3 {};
};

Is it okay?

[HazardyKnusperkeks]

Apart from that comment I don't have any objections. :)

clang/include/clang/Format/Format.h

2025–2033

You don't write the .. code-block your self.

[anastasiia_lukianenko]

@curdeius could you please write your opinion?

[curdeius]

clang/unittests/Format/FormatTest.cpp

3526

Hey, your test looks good. Sorry for the delay.

[anastasiia_lukianenko]

@MyDeveloperDay @HazardyKnusperkeks could you please clarify is there something I need to change in patch or we can push it to the next stage?

[HazardyKnusperkeks]

For me it's good, but please wait for @MyDeveloperDay .

[MyDeveloperDay]

Did you prove that CLASS(x) : won't be an issue?

or

return  x > y ? FOO(x) : BAR(y)

[MyDeveloperDay]

class FOO(x) : public x {
  public:
    int x;

    FOO(int y) : x(y) {}
};

will become

class FOO(x) : public x {
  public:
    int x;

FOO(int y) :
    x(y) {}
};

with this patch.

[anastasiia_lukianenko]

In D92168#2774060, @MyDeveloperDay wrote:

Did you prove that CLASS(x) : won't be an issue?

or

return  x > y ? FOO(x) : BAR(y)

I will add test for it soon. I checked these cases locally and no issue found.

[anastasiia_lukianenko]

@MyDeveloperDay could you please clarify why unit tests are failed on Phabricator and there is no diff in logs (what is expected), but locally I tested the same tests and the result was successful?

[MyDeveloperDay]

In D92168#2777670, @anastasiia_lukianenko wrote:

@MyDeveloperDay could you please clarify why unit tests are failed on Phabricator and there is no diff in logs (what is expected), but locally I tested the same tests and the result was successful?

These tests fail for me with your current revision.

[anastasiia_lukianenko]

@MyDeveloperDay How can I see the formatted result to compare it with expected (to know what exactly wrong)? Now I see only these in logs on Phabricator:

Script:
--
/var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/unittests/Format/./FormatTests --gtest_filter=FormatTest.StatementMacroInSwitchBlock
--
Note: Google Test filter = FormatTest.StatementMacroInSwitchBlock
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from FormatTest
[ RUN      ] FormatTest.StatementMacroInSwitchBlock
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:45: Failure
Expected equality of these values:
  ExpectedCompleteFormat
    Which is: true
  Status.FormatComplete
    Which is: false
class FOO(x) : public x {
public:
  int x;
  FOO(int y) : x(y) {}
};


Google Test trace:
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:3864: class FOO(x) : public x { public: int x; FOO(int y) : x(y) {} };
/var/lib/buildkite-agent/builds/llvm-project/clang/unittests/Format/FormatTest.cpp:45: Failure
Expected equality of these values:
  ExpectedCompleteFormat
    Which is: true
  Status.FormatComplete
    Which is: false
class FOO(x) : public x { public: int x; FOO(int y) : x(y) {} };

[anastasiia_lukianenko]

@MyDeveloperDay after my research I found that tests fail because the status of lines below formatting are marked as FormatComplete = false.

FOO(int y) : x(y) {}\n"
"return x > y ? FOO(x) : BAR(y);"

It means:

/// A value of ``false`` means that any of the affected ranges were not
  /// formatted due to a non-recoverable syntax error.

Does it mean that the test itself is marked as invalid?

[MyDeveloperDay]

I'm not sure of the reason for the failure without debugging it, We cannot commit with failing tests so we have to determine the reason why.

[MyDeveloperDay]

clang/lib/Format/UnwrappedLineParser.cpp

2920

Can you comment by what you mean by "InlevelParsing"?

[anastasiia_lukianenko]

In D92168#2781276, @MyDeveloperDay wrote:

I'm not sure of the reason for the failure without debugging it, We cannot commit with failing tests so we have to determine the reason why.

I checked this test without my patch and I have the same result. I think I need to understand why this test cause a syntax error firstly.

[MyDeveloperDay]

if you think this is done you need to mark the comments as done if you have addressed them

[HazardyKnusperkeks]

More than one year silence.

clang/include/clang/Format/Format.h

   /// A vector of macros that should be interpreted as complete
   /// statements.
   ///
   /// Typical macros are expressions, and require a semi-colon to be
   /// added; sometimes this is not the case, and this allows to make
   /// clang-format aware of such cases.
   ///
   /// For example: Q_UNUSED
+  ///
+  /// Cases in switch block can be defined as StatementMacros. The behavior is
+  /// the same as 'case' in switch block.
+  ///
+  /// .. code-block:: c
+  ///   switch (x) {
+  ///   FOO:
+  ///     break;
+  ///   case 0:
+  ///     break;
+  ///   BAR(var) :
+  ///     break;
+  ///   }
   std::vector<std::string> StatementMacros;
 
   /// A vector of macros which are used to open namespace blocks.
   ///
   /// These are expected to be macros of the form:
   /// \code
   ///   NAMESPACE(<namespace-name>, ...) {
   ///     <namespace-content>

clang/lib/Format/UnwrappedLineParser.h

private:
   void parsePPDirective();
   void parsePPDefine();
   void parsePPIf(bool IfDef);
   void parsePPElIf();
   void parsePPElse();
   void parsePPEndIf();
   void parsePPUnknown();
   void readTokenWithJavaScriptASI();
-  void parseStructuralElement();
+  void parseStructuralElement(bool inLevelParsing = false);
   bool tryToParseBracedList();
   bool parseBracedList(bool ContinueOnSemicolons = false, bool IsEnum = false,
                        tok::TokenKind ClosingBraceKind = tok::r_brace);
   void parseParens();
   void parseSquare(bool LambdaIntroducer = false);
   void parseIfThenElse();
   void parseTryCatch();
   void parseForOrWhileLoop();
private:
   void parseRecord(bool ParseAsExpr = false);
   void parseObjCLightweightGenerics();
   void parseObjCMethod();
   void parseObjCProtocolList();
   void parseObjCUntilAtEnd();
   void parseObjCInterfaceOrImplementation();
   bool parseObjCProtocol();
   void parseJavaScriptEs6ImportExport();
-  void parseStatementMacro();
+  void parseStatementMacro(bool inLevelParsing = false);
   void parseCSharpAttribute();
   // Parse a C# generic type constraint: `where T : IComparable<T>`.
   // See:
   // https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/keywords/where-generic-type-constraint
   void parseCSharpGenericTypeConstraint();
   bool tryToParseLambda();
   bool tryToParseLambdaIntroducer();
   bool tryToParsePropertyAccessor();

clang/lib/Format/UnwrappedLineParser.cpp

void UnwrappedLineParser::parseLevel(bool HasOpeningBrace) {
   bool SwitchLabelEncountered = false;
   do {
     tok::TokenKind kind = FormatTok->Tok.getKind();
     if (FormatTok->getType() == TT_MacroBlockBegin) {
       kind = tok::l_brace;
     } else if (FormatTok->getType() == TT_MacroBlockEnd) {
       kind = tok::r_brace;
     }
-
+    if (FormatTok->is(TT_StatementMacro))
+      parseStructuralElement(/*inLevelParsing=*/true);
     switch (kind) {
     case tok::comment:
       nextToken();
       addUnwrappedLine();
       break;
     case tok::l_brace:
       // FIXME: Add parameter whether this can happen - if this happens, we must
       // be in a non-declaration context.
if (NextMustBeValue && !NextEndsTemplateExpr && !PreviousStartsTemplateExpr &&
        Previous->isOneOf(tok::r_square, tok::r_paren, tok::plusplus,
                          tok::minusminus)))
     return addUnwrappedLine();
   if ((PreviousMustBeValue || Previous->is(tok::r_paren)) &&
       isJSDeclOrStmt(Keywords, Next))
     return addUnwrappedLine();
 }
 
-void UnwrappedLineParser::parseStructuralElement() {
+void UnwrappedLineParser::parseStructuralElement(bool inLevelParsing) {
   assert(!FormatTok->is(tok::l_brace));
   if (Style.Language == FormatStyle::LK_TableGen &&
       FormatTok->is(tok::pp_include)) {
     nextToken();
     if (FormatTok->is(tok::string_literal))
       nextToken();
     addUnwrappedLine();
     return;
   }
   switch (FormatTok->Tok.getKind()) {
   case tok::kw_asm:
     nextToken();
     if (FormatTok->is(tok::l_brace)) {
       FormatTok->setType(TT_InlineASMBrace);
       nextToken();
       while (FormatTok && FormatTok->isNot(tok::eof)) {
         if (FormatTok->is(tok::r_brace)) {
           FormatTok->setType(TT_InlineASMBrace);
if (Style.isCpp() &&
       nextToken();
       if (FormatTok->is(tok::colon)) {
         nextToken();
         addUnwrappedLine();
         return;
       }
     }
     if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
-      parseStatementMacro();
+      parseStatementMacro(inLevelParsing);
       return;
     }
     if (Style.isCpp() && FormatTok->is(TT_NamespaceMacro)) {
       parseNamespace();
       return;
     }
     // In all other cases, parse the declaration.
     break;
if (FormatTok->is(tok::l_brace)) {
       nextToken();
       parseBracedList();
     } else {
       nextToken();
     }
   }
 }
 
-void UnwrappedLineParser::parseStatementMacro() {
+void UnwrappedLineParser::parseStatementMacro(bool inLevelParsing) {
   nextToken();
   if (FormatTok->is(tok::l_paren))
     parseParens();
+  if (inLevelParsing && FormatTok->is(tok::colon))
+    parseLabel();
   if (FormatTok->is(tok::semi))
     nextToken();
   addUnwrappedLine();
 }
 
 LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine &Line,
                                                  StringRef Prefix = "") {
   llvm::dbgs() << Prefix << "Line(" << Line.Level

clang/unittests/Format/FormatTest.cpp

 TEST_F(FormatTest, LayoutMacroDefinitionsStatementsSpanningBlocks) {
   verifyFormat("#define A \\\n"
                "  f({     \\\n"
                "    g();  \\\n"
                "  });",
                getLLVMStyleWithColumns(11));
 }
 
+TEST_F(FormatTest, StatementMacroInSwitchBlock) {
+  FormatStyle Style = getLLVMStyle();
+  Style.StatementMacros.push_back("FOO");
+  verifyFormat("switch (x) {\n"
+               "FOO:\n"
+               "  break;\n"
+               "case 0:\n"
+               "  break;\n"
+               "FOO(var) :\n"
+               "  break;\n"
+               "case 1:\n"
+               "  break;\n"
+               "}",
+               Style);
+  /* This option removes the space before the StatementMacro colon */
+  Style.SpaceBeforeCtorInitializerColon = false;
+  verifyFormat("switch (x) {\n"
+               "FOO:\n"
+               "  break;\n"
+               "case 0:\n"
+               "  break;\n"
+               "FOO(var):\n"
+               "  break;\n"
+               "case 1:\n"
+               "  break;\n"
+               "}",
+               Style);
+}
+
 TEST_F(FormatTest, IndentPreprocessorDirectives) {
   FormatStyle Style = getLLVMStyle();
   Style.IndentPPDirectives = FormatStyle::PPDIS_None;
   Style.ColumnLimit = 40;
   verifyFormat("#ifdef _WIN32\n"
                "#define A 0\n"
                "#ifdef VAR2\n"
                "#define B 1\n"