This is an archive of the discontinued LLVM Phabricator instance.

[clang-format] Don't break lines after pragma region
ClosedPublic

Authored by thieta on May 19 2022, 3:25 AM.

Details

Summary

We have autogenerated pragma regions in our code
which where awkwardly broken up like this:

#pragma region foo(bar : hello)

becomes

#pragma region foo(bar \
                   : hello)

This fixes the problem by adding region as a keyword
and handling it the same way as pragma mark

Diff Detail

Event Timeline

thieta created this revision.May 19 2022, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 3:25 AM
thieta requested review of this revision.May 19 2022, 3:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 3:25 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
thieta edited the summary of this revision. (Show Details)May 19 2022, 3:26 AM
thieta added a project: Restricted Project.
thieta added a subscriber: Restricted Project.

There are other pragmas which include colon. How do they fare?

Example https://docs.microsoft.com/en-us/cpp/preprocessor/warning?view=msvc-170

#pragma warning( disable : 4507 34; once : 4385; error : 164 )
thieta updated this revision to Diff 430882.May 19 2022, 10:56 PM

Added additional tests to check other pragma statements

There are other pragmas which include colon. How do they fare?

Example https://docs.microsoft.com/en-us/cpp/preprocessor/warning?view=msvc-170

#pragma warning( disable : 4507 34; once : 4385; error : 164 )

I checked this and added an additional test to test for this. It works fine without any changes - I think the difference is when there is a additional token between pragma and the "text".

curdeius requested changes to this revision.May 20 2022, 4:59 AM

Thanks for the patch!
Could you please add a unit test in unittest/Format/FormatTest.cpp instead of in lit-based test/?
Is there a bug report that your patch fixes?

clang/lib/Format/TokenAnnotator.cpp
1228

Please fix (remove) this comment that consumes not only mark.

This revision now requires changes to proceed.May 20 2022, 4:59 AM

Thanks for the patch!
Could you please add a unit test in unittest/Format/FormatTest.cpp instead of in lit-based test/?
Is there a bug report that your patch fixes?

Sure - I didn't know clang-format used unittests instead. Maybe there should be some documentation around this for new people used to adding lit tests?

I never filed a bug for it - I fixed it instead.

Thanks

Thanks for the patch!
Could you please add a unit test in unittest/Format/FormatTest.cpp instead of in lit-based test/?
Is there a bug report that your patch fixes?

Sure - I didn't know clang-format used unittests instead.

Yes, non-unit tests serve meeting for the config tests IIRC.

Maybe there should be some documentation around this for new people used to adding lit tests?

That would certainly help. Feel free to do it if you
Otherwise I'll do it but only later (in ten days or so).

thieta updated this revision to Diff 430953.May 20 2022, 6:10 AM

Switched to unit test over lit test

thieta updated this revision to Diff 430954.May 20 2022, 6:12 AM

Removed comment

thieta marked an inline comment as done.May 20 2022, 6:12 AM
curdeius accepted this revision.May 20 2022, 6:20 AM

Thanks! LGTM.

This revision is now accepted and ready to land.May 20 2022, 6:20 AM
This revision was landed with ongoing or failed builds.May 20 2022, 7:11 AM
This revision was automatically updated to reflect the committed changes.
MyDeveloperDay added inline comments.May 23 2022, 8:53 AM
clang/unittests/Format/FormatTest.cpp
18832

do we need to consider endregion at all? it would be nice to have some tests that included both.