Comment alignment regression

[sam-mccall]

#if 0
#endif

#if X
int something_fairly_long;  // Align here please
#endif                      // Should be aligned

This regressed with 2183fe2

[llvmbot]

@llvm/issue-subscribers-clang-format

[sam-mccall]

Another example, this time between directives:

#if 0
#endif

#if X
#else   // Align
;
#endif  // Align

[eywdck2l]

I found the reason, but the fix may take some work.

The array PPLevelBranchIndex is supposed to be the same throughout an entire pass of the file. It is not popped when a conditional part is over. So in the first example, 1 is pushed when the parser sees the first #if 0, then in #if X the branch index is still 1. So everything in the #if X block is skipped over. A way to fix this particular problem is https://reviews.llvm.org/D135372.

However, I found this bigger problem that has always existed when investigating. I guess its fix will overwrite my patch. Keeping things simple and assuming none of the preprocessor branches are unreachable, the first branches of all blocks are parsed on the first run, no problem here. But on the second run, the second branches are parsed, and a block is skipped if it only has one branch. In the following code, the second branch of the second block is not indented because the first block only has one branch.

#if 1
{
#endif
#if X
  x;
#else
x;
#endif
}

Does anyone know how to fix this problem?

[eywdck2l]

@HazardyKnusperkeks Can you fix the problem I mentioned?

[HazardyKnusperkeks]

@HazardyKnusperkeks Can you fix the problem I mentioned?

Sorry, never worked with the preprocessor stuff in clang-format.

[sam-mccall]

If nobody is able to fix it, should we consider reverting?

Heuristics to parse the correct branch are useful, but the branch index is "leaking" outside its original context and it's very common that conditionals only have a single branch, so this seems likely to be a net loss.

The nonlocal behavior of this bug feels flaky in a way that the original behavior didn't (just a bit "dumb").

[eywdck2l]

Now I have a fix.

https://reviews.llvm.org/D135740

[owenca]

However, I found this bigger problem that has always existed when investigating. I guess its fix will overwrite my patch. Keeping things simple and assuming none of the preprocessor branches are unreachable, the first branches of all blocks are parsed on the first run, no problem here. But on the second run, the second branches are parsed, and a block is skipped if it only has one branch. In the following code, the second branch of the second block is not indented because the first block only has one branch.

#if 1
{
#endif
#if X
  x;
#else
x;
#endif
}

@eywdck2l Can you create a separate issue for this? As nobody can review your patch yet, I'm not sure if it's worth fixing if we can't find a simpler way.

[eywdck2l]

Why can nobody review my patch yet? Are you too busy?

[owenca]

Why can nobody review my patch yet?

Because none of us (code owners and main reviewers) is familiar enough with this part of the code.