[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

1032

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 ↗

(On Diff #337430)

Is this needed?

[anastasiia_lukianenko]

clang/unittests/Format/FormatTest.cpp

3502 ↗

(On Diff #337430)

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

2757

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 ↗

(On Diff #338960)

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

2757

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 ↗

(On Diff #337430)

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

[anastasiia_lukianenko]

clang/unittests/Format/FormatTest.cpp

3503 ↗

(On Diff #338960)

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

[HazardyKnusperkeks]

clang/unittests/Format/FormatTest.cpp

3503 ↗

(On Diff #338960)

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

1022

Nit: please use CamelCase for parameters and local variables.

1191

2752

clang/unittests/Format/FormatTest.cpp

3526 ↗

(On Diff #343408)

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 ↗

(On Diff #343408)

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 ↗

(On Diff #343408)

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 ↗

(On Diff #346080)

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 ↗

(On Diff #343408)

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

2752

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/lib/Format/UnwrappedLineParser.cpp

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() {
   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;
   }
+  if (FormatTok->is(TT_StatementMacro)) {
+    parseCaseLabel();
+    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)) {
if (Style.isCpp() &&
       nextToken();
       if (FormatTok->is(tok::colon)) {
         nextToken();
         addUnwrappedLine();
         return;
       }
     }
     if (Style.isCpp() && FormatTok->is(TT_StatementMacro)) {
       parseStatementMacro();
       return;
     }
     if (Style.isCpp() && FormatTok->is(TT_NamespaceMacro)) {
       parseNamespace();
       return;
     }
     // In all other cases, parse the declaration.
     break;
void UnwrappedLineParser::parseLabel(bool LeftAlignLabel) {
   Line->Level = OldLineLevel;
   if (FormatTok->isNot(tok::l_brace)) {
     parseStructuralElement();
     addUnwrappedLine();
   }
 }
 
 void UnwrappedLineParser::parseCaseLabel() {
-  assert(FormatTok->Tok.is(tok::kw_case) && "'case' expected");
+  assert((FormatTok->Tok.is(tok::kw_case) && "'case' expected") ||
+         FormatTok->is(TT_StatementMacro));
   // FIXME: fix handling of complex expressions here.
   do {
     nextToken();
   } while (!eof() && !FormatTok->Tok.is(tok::colon));
   parseLabel();
 }
 
 void UnwrappedLineParser::parseSwitch() {
if (FormatTok->is(tok::l_brace)) {
       nextToken();
       parseBracedList();
     } else {
       nextToken();
     }
   }
 }
 
 void UnwrappedLineParser::parseStatementMacro() {
   nextToken();
   if (FormatTok->is(tok::l_paren))
     parseParens();
   if (FormatTok->is(tok::semi))
     nextToken();
   addUnwrappedLine();
 }
 
 LLVM_ATTRIBUTE_UNUSED static void printDebugInfo(const UnwrappedLine &Line,
                                                  StringRef Prefix = "") {
   llvm::dbgs() << Prefix << "Line(" << Line.Level
                << ", FSC=" << Line.FirstStartColumn << ")"
                << (Line.InPPDirective ? " MACRO" : "") << ": ";