This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Discard pre-processor statements in parseBracedList()
AbandonedPublic

Authored by thieta on Oct 20 2022, 4:44 AM.

Details

Summary

We had some code that looked like this:

int foo()
{
  #pragma region hello(foo)
  #pragma endregion
}

foo* bar() {}

This was confusingly indented like:

int foo()
  {
    #pragma region...
  }

After some investigation I noticed that this is because
parseBracedList() thought that this was a initializer list.

The check here: https://github.com/tru/llvm-project/blob/main/clang/lib/Format/UnwrappedLineParser.cpp#L709
will mark the code above as ProbablyBracedList and then it
will format it totally wrong depending on your style settings.

My initial fix was to change the check above, but it became
really complicated to keep both initializer lists and my code
working.

My approach here instead is to discard any line that starts with #
since that is a pre-processor statement we shouldn't really care
about it in this case.

This fix passes all the unittests and our internal code-base, so
I am fairly confident in it, but I am no clang-format expert.

Diff Detail

Event Timeline

thieta created this revision.Oct 20 2022, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:44 AM
thieta requested review of this revision.Oct 20 2022, 4:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 20 2022, 4:44 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Can you create an issue on GitHub and include the details on how to reproduce the problem using the latest clang-format?

clang/unittests/Format/FormatTest.cpp
19975–19978

Can you clean up the tests a little? i.e. use verifyFormat() and have one string literal per test case line?

thieta abandoned this revision.Oct 24 2022, 11:36 PM

Can you create an issue on GitHub and include the details on how to reproduce the problem using the latest clang-format?

Ah I re-tested this with main and it seems like there has been a lot of changes to parseBracedList() since I started to investigate this and it seems to work out of the box now. I will close this diff.