Page MenuHomePhabricator

[clang-format] Support custom If macros
Needs ReviewPublic

Authored by vlovich on Tue, May 18, 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.Tue, May 18, 3:29 PM
vlovich created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptTue, May 18, 3:29 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
vlovich added inline comments.Tue, May 18, 3:29 PM
clang/include/clang/Format/Format.h
2098

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
2098

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.Thu, May 20, 2:50 PM
vlovich added inline comments.
clang/include/clang/Format/Format.h
2098

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.Fri, May 21, 9:04 AM
clang/docs/ReleaseNotes.rst
252

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
3025

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

clang/unittests/Format/FormatTest.cpp
19703

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
252

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
3025

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
2983

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.Thu, May 27, 12:56 PM
clang/include/clang/Format/Format.h
2983

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
19719

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.