Page MenuHomePhabricator

[clang-format] clang-format off/on not respected when using C Style comments
ClosedPublic

Authored by MyDeveloperDay on Mar 1 2019, 4:33 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

MyDeveloperDay created this revision.Mar 1 2019, 4:33 AM
alexfh added a subscriber: alexfh.Mar 1 2019, 6:09 AM
alexfh added inline comments.
clang/unittests/Format/SortIncludesTest.cpp
132 ↗(On Diff #188880)

Add a test with /* clang-format officially supports C++ */ ;)

Seriously speaking, the startswith() condition in the code should be a bit stricter. Maybe just compare with /* clang-format off */ and /* clang-format on */? If there's a motivating use case for just checking the prefix, could you add it to the test?

MyDeveloperDay marked an inline comment as done.Mar 1 2019, 6:49 AM
MyDeveloperDay added inline comments.
clang/unittests/Format/SortIncludesTest.cpp
132 ↗(On Diff #188880)

You know i thought I was being so clever by not having the space after ..off to avoid

/*clang-format off*/

That'll teach me!

Fix negative test case
support the same /*<space>clang-format off<space>*/ in the sort includes that the TokenLexer supports.

MyDeveloperDay marked an inline comment as done.Mar 1 2019, 9:48 AM
JonasToth added inline comments.Mar 1 2019, 10:21 AM
clang/lib/Format/Format.cpp
1792 ↗(On Diff #188931)

Should we allow

/* clang-format off
    It is just horrible for this piece of code. */

? Multiline-comments could span multiple lines and to deactivates clang-format and give reasons.

MyDeveloperDay marked an inline comment as done.Mar 1 2019, 12:36 PM
MyDeveloperDay added inline comments.
clang/lib/Format/Format.cpp
1792 ↗(On Diff #188931)

initially that is kind of what I was trying to do, but if you look over in TokenLexer where its turned off for styling they check only for a single line

So to be honest if we are going to change it we should change it in both places, but perhaps that is just overkill.

JonasToth added inline comments.Mar 1 2019, 12:41 PM
clang/lib/Format/Format.cpp
1792 ↗(On Diff #188931)

Totally, not necessary in my eyes, nice plus, so dont implement it :D

MyDeveloperDay marked 2 inline comments as done.Mar 1 2019, 1:13 PM
This revision is now accepted and ready to land.Mar 1 2019, 2:52 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2019, 1:08 AM