[PATCH] D102730: [clang-format] Support custom If macros
Vitali Lovich via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri May 21 16:26:31 PDT 2021
vlovich added inline comments.
================
Comment at: clang/docs/ReleaseNotes.rst:252
+- Option ``IfMacros`` has been added. This lets you define macros that get
+ formatted like conditionals much like ``ForEachMacros`` get stiled like
+ foreach loops.
----------------
MyDeveloperDay wrote:
> stiled? did you mean styled?
Whoops. Yes I did.
================
Comment at: clang/lib/Format/FormatTokenLexer.cpp:1019
FormatTok->setType(it->second);
+ if (it->second == TT_IfMacro) {
+ FormatTok->Tok.setKind(tok::kw_if);
----------------
MyDeveloperDay wrote:
> Is there any chance you could leave a comment here as to why this is needed, I can't work it out?
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.
================
Comment at: clang/lib/Format/TokenAnnotator.cpp:3025
return false;
+ if (Left.is(TT_IfMacro)) {
+ return false;
----------------
MyDeveloperDay wrote:
> I'll let you decide if you think we need another SBPO_XXX style?
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D102730/new/
https://reviews.llvm.org/D102730
More information about the cfe-commits
mailing list