[clang-format] Don't skip #else/#elif of #if 0

[owenpan]

Summary

Fixes https://github.com/llvm/llvm-project/issues/58188.

[sstwcw]

This patch fixes the regression caused by 2183fe2 while introducing a new regression. But in my opinion the new regression is less of a problem than the old one. Therefore I think it is okay.

Take this piece of code similar to the example in 71814b4.

before

void SomeFunction(int param1,
                  template <
#ifdef A
#if 0
#endif
                      MyType<Some>>
#else
                      Type1, Type2>
#endif
                  param2,
                  param3) {
  f();
}

after

void SomeFunction(int param1,
                  template <
#ifdef A
#if 0
#endif
                      MyType<Some>>
#else
                  Type1,
                  Type2 >
#endif
                      param2,
                  param3) {
  f();
}

clang/lib/Format/UnwrappedLineParser.cpp

1150

Can you add a comment here saying this line is a nasty hack which breaks the assumption that PPChainBranchIndex should have as many entries as the number of nestings of preprocessor branches and we should probably come up with a better way?

[owenpan]

Tweaked the fix and added a test.

[owenpan]

In D137052#3895426, @sstwcw wrote:

This patch fixes the regression caused by 2183fe2 while introducing a new regression. But in my opinion the new regression is less of a problem than the old one. Therefore I think it is okay.

Take this piece of code similar to the example in 71814b4.

before

void SomeFunction(int param1,
                  template <
#ifdef A
#if 0
#endif
                      MyType<Some>>
#else
                      Type1, Type2>
#endif
                  param2,
                  param3) {
  f();
}

after

void SomeFunction(int param1,
                  template <
#ifdef A
#if 0
#endif
                      MyType<Some>>
#else
                  Type1,
                  Type2 >
#endif
                      param2,
                  param3) {
  f();
}

Fixed.

[sstwcw]

Thanks for fixing the problem I caused.

[aaron.ballman]

Can you also add a test for #elifdef and #elifndef?

[sstwcw]

It just occurred to me that klimek is the one who added all this stuff. Why didn't you include him as a reviewer?

[MyDeveloperDay]

In D137052#3899094, @sstwcw wrote:

It just occurred to me that klimek is the one who added all this stuff. Why didn't you include him as a reviewer?

@djasper and @klimek name is likely all over the code, as they were the "founding fathers of clang-format", hopefully they are off doing something new and exciting, but those of us here on the reviewers list along with @curdeius are really the core clang-format team at this time. Hopefully we are qualified enough. (actually @owenpan is likely the most qualified ;-) )

We do our best to stay true to the original designers goals and be good custodians of clang-format, but my hope is they are watching over us even from afar but we assume they trust us with their baby!

We tend not to ask them to give back more unless they want to.

[owenpan]

In D137052#3899091, @aaron.ballman wrote:

Can you also add a test for #elifdef and #elifndef?

I added a test (lines 6130-6136) for #elif but not #elifdef and #elifndef because all three are on the same execution path in the parser (lines 1113-1115). Actually, they all call the same function (via parsePPElIf()) that #else does. This is probably why none of them has a separate test in the LayoutStatementsAroundPreprocessorDirectives unit test. So I will do the opposite and remove the test for #elif. :)

clang/lib/Format/UnwrappedLineParser.cpp

if (Unreachable ||
     PPStack.push_back({PP_Conditional, Line});
   }
 }
 
 void UnwrappedLineParser::conditionalCompilationStart(bool Unreachable) {
   ++PPBranchLevel;
   assert(PPBranchLevel >= 0 && PPBranchLevel <= (int)PPLevelBranchIndex.size());
   if (PPBranchLevel == (int)PPLevelBranchIndex.size()) {
-    // If the first branch is unreachable, set the BranchIndex to 1.  This way
-    // the next branch will be parsed if there is one.
-    PPLevelBranchIndex.push_back(Unreachable ? 1 : 0);
+    PPLevelBranchIndex.push_back(0);
     PPLevelBranchCount.push_back(0);
   }
-  PPChainBranchIndex.push(0);
+  PPChainBranchIndex.push(Unreachable ? -1 : 0);
   bool Skip = PPLevelBranchIndex[PPBranchLevel] > 0;
   conditionalCompilationCondition(Unreachable || Skip);
 }
 
 void UnwrappedLineParser::conditionalCompilationAlternative() {
   if (!PPStack.empty())
     PPStack.pop_back();
   assert(PPBranchLevel < (int)PPLevelBranchIndex.size());

clang/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, LayoutStatementsAroundPreprocessorDirectives) {
   // `#else`.
   verifyFormat("#if 0\n"
                "{\n"
                "#else\n"
                "{\n"
                "#endif\n"
                "  x;\n"
                "}");
+
+  verifyFormat("#if 0\n"
+               "#endif\n"
+               "#if X\n"
+               "int something_fairly_long; // Align here please\n"
+               "#endif                     // Should be aligned");
+
+  verifyFormat("#if 0\n"
+               "#endif\n"
+               "#if X\n"
+               "#else  // Align\n"
+               ";\n"
+               "#endif // Align");
+
+  verifyFormat("void SomeFunction(int param1,\n"
+               "                  template <\n"
+               "#ifdef A\n"
+               "#if 0\n"
+               "#endif\n"
+               "                      MyType<Some>>\n"
+               "#else\n"
+               "                      Type1, Type2>\n"
+               "#endif\n"
+               "                  param2,\n"
+               "                  param3) {\n"
+               "  f();\n"
+               "}");
 }
 
 TEST_F(FormatTest, GraciouslyHandleIncorrectPreprocessorConditions) {
   verifyFormat("#endif\n"
                "#if B");
 }
 
 TEST_F(FormatTest, FormatsJoinedLinesOnSubsequentRuns) {