Page MenuHomePhabricator

[clang-format] Support custom If macros
ClosedPublic

Authored by vlovich on May 18 2021, 3:29 PM.

Details

Summary

Like with ForEachMacros, support user to provide additional macros that act like if statements.

This makes it possible to support KJ_IF_MAYBE from the cap'n'proto library:
https://github.com/capnproto/capnproto/blob/master/kjdoc/tour.md#maybes.

https://bugs.llvm.org/show_bug.cgi?id=49354

Diff Detail

Event Timeline

vlovich requested review of this revision.May 18 2021, 3:29 PM
vlovich created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMay 18 2021, 3:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vlovich added inline comments.May 18 2021, 3:29 PM
clang/include/clang/Format/Format.h
2136

Looks good, but please add a test with the else IF and just else case.

Also for me it would be ok to add your KJ_IF_MAYBE to the default configuration. But I have no experience on that.

clang/include/clang/Format/Format.h
2136

Maybe. *scnr*

Review response:

  • Added additional tests.
  • Added KJ_IF_MAYBE to default IfMacros.
  • Added links to KJ documentation.
vlovich marked an inline comment as done.May 20 2021, 2:50 PM
vlovich added inline comments.
clang/include/clang/Format/Format.h
2136

Well done. Took me a second.

vlovich marked an inline comment as done.

Updated release notes.

Fixed missing _ after the hyperlink to the KJ link in the Style options documentation. I'm assuming that's required formatting for hyperlinks by what renders the markdown.

MyDeveloperDay added inline comments.May 21 2021, 9:04 AM
clang/docs/ReleaseNotes.rst
260

stiled? did you mean styled?

clang/lib/Format/FormatTokenLexer.cpp
1019

Is there any chance you could leave a comment here as to why this is needed, I can't work it out?

clang/lib/Format/TokenAnnotator.cpp
3126

I'll let you decide if you think we need another SBPO_XXX style?

clang/unittests/Format/FormatTest.cpp
21036

this should support `MYIF<space>(` by processing SpacesInConditionalStatement as we do with FOREACH

vlovich marked 3 inline comments as done.

Review response. Updated the options I changed via the dump_format_style.py script but didn't ingest all the other changes it generated.
Changed the SBPO option to have a more generic name that also applies to IF macros.
Fix typo in release notes.

clang/docs/ReleaseNotes.rst
260

Whoops. Yes I did.

clang/lib/Format/FormatTokenLexer.cpp
1019

Will do my best. TLDR: The token is otherwise "unknown" & the conditional formatting logic depends on the lexer token value. This avoids having to rewrite all the existing code & seemed to make sense since we want this token treated like an if.

I don't of course know if this is The Best Way (tm). It's just the minimal change I figured out would get the formatting to work properly but happy to apply feedback from someone more intimately familiar with the internals of the formatter as I'm a complete novice here.

clang/lib/Format/TokenAnnotator.cpp
3126

I thought about it but I wasn't was really sure how to add it in a way that would make sense. Do you think people would want to apply consistent SBPO styling for IF & FOREACH macros or want fine-grained control? If the former, then I can just check the foreach macro & maybe rename it to SBPO_ControlStatementsExceptMacros (maintaining the old name for back compat). If the latter, then it would seem like we need a separate boolean that controls whether SBPO_ControlStatements would apply?

My gut is probably the "maintain consistency" option is fine for now so I've gone ahead & applied that change in the latest diff.

clang/include/clang/Format/Format.h
3067

Why did you change this?

This kind of looks ok to me, I'm not sure about the "lexer" part, I guess I don't know the impact but I'd expect it to be small because it's conditional on your IfMacro

I'd like @curdeius or @krasimir to take a look.

vlovich added inline comments.May 27 2021, 12:56 PM
clang/include/clang/Format/Format.h
3067

Per the discussion below.

MyDeveloperDay
I'll let you decide if you think we need another SBPO_XXX style?

Me
I thought about it but I wasn't was really sure how to add it in a way that would make sense. Do you think people would want to apply consistent SBPO styling for IF & FOREACH macros or want fine-grained control? If the former, then I can just check the foreach macro & maybe rename it to SBPO_ControlStatementsExceptMacros (maintaining the old name for back compat). If the latter, then it would seem like we need a separate boolean that controls whether SBPO_ControlStatements would apply?
My gut is probably the "maintain consistency" option is fine for now so I've gone ahead & applied that change in the latest diff.

This felt like a simpler solution because otherwise you would either end up with SBPO_ControlStatementsForEachMacros, SBPO_ControlStatementsExceptIfAndForEachMacros, SBPO_ControlStatementsExceptIfMacros which just feels extremely confusing (& for now I'm assuming you'll want a similar style for ForEach & If macros). Arguably at the point where you want distinct SBPO styling of these control-like macros, you would be moved them out into a separate option since it's really orthogonal to the other settings.

Open to suggestions of course.

Thanks for working on this!
Looks pretty nice, but please clang-format the changes and add some more test cases (cf. inline comment).

Concerning the lexer part, I wouldn't do anything cleaner.

clang/unittests/Format/FormatTest.cpp
21052

Please add a test to see the interaction with AllowShortIfStatementsOnASingleLine, and maybe BreakBeforeBraces, BraceWrappingAfterControlStatementStyle, SpacesInConditionalStatement.

Just back from vacation so will try to polish this off today.

Review response

I've updated coverage for AllowShortIfStatementsOnASingleLine and
BreakBeforeBraces. SpacesInConditionalStatement is what the existing
tests were covering so I don't think there's anything there to update.
I looked & couldn't find any tests for
BraceWrappingAfterControlStatementStyle so my (admittedly lazy)
preference is to skip that for now if possible.

vlovich marked an inline comment as done.

Fix clang-format issue

Seems to be okay for me.

This revision is now accepted and ready to land.Jun 22 2021, 11:42 AM

Rebase onto main.

This revision was landed with ongoing or failed builds.Jun 23 2021, 8:53 AM
This revision was automatically updated to reflect the committed changes.