[PATCH] D102730: [clang-format] Support custom If macros

[Vitali Lovich via Phabricator via cfe-commits]

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