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

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to