[PATCH] D144170: [clang-format] Add simple macro replacements in formatting.

Thu Feb 16 05:43:48 PST 2023

sammccall added a comment.

It's happening!

================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:743

+  // Align following lines within parenthesis / brackets if configured.
+  // For a line of macro parents, the commas that follow the opening parenthesis
----------------
nit: parentheses

================
Comment at: clang/lib/Format/ContinuationIndenter.cpp:744
+  // Align following lines within parenthesis / brackets if configured.
+  // For a line of macro parents, the commas that follow the opening parenthesis
+  // in the line come after the opening parenthesis' children - we want to align
----------------
This is a bit hard to follow without context, I had to look up MacroCallReconstructor::takeResult() to recall the structure it's referring to, and there's no obvious breadcrumb to point you there. And if you don't follow the details, it's hard to understand what the relevance is.

Maybe something like: this doesn't apply to macro expansion lines, which are `MACRO( , , )` with args as children of `(` and `,`. It does not make sense to align the commas with the opening paren.

(I think describing here how the alignment ends up working in that case might just add confusion: it's not handled by this code, right?)

================
Comment at: clang/lib/Format/TokenAnnotator.h:68
     FormatToken *Current = First;
+    addChildren(Line.Tokens.front(), Current);
     for (const UnwrappedLineNode &Node : llvm::drop_begin(Line.Tokens)) {
----------------
this seems like a separate bugfix: previously if the first node in Line.Tokens had children, we wouldn't be adding them.

Is this true? Was there some reason the first node could never have children before?

================
Comment at: clang/lib/Format/TokenAnnotator.h:76
+      addChildren(Node, Current);
+      // FIXME: if we add children, previous will point to the token before
+      // the children; changing this requires significant changes across
----------------
does "add children" here refer to the procedure in addChildren(), or to the token insertion idea introduced in the previous patch?

(i.e. is this just describing that previous/next skip over trees of children rather than including them in the chain, or is there something new going on?)

================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:924
+      Tok->MacroCtx->Role = MR_UnexpandedArg;
+      Tok->SpacesRequiredBefore = 0;
+    } else {
----------------
comment as to why?

================
Comment at: clang/lib/Format/UnwrappedLineFormatter.cpp:926
+    } else {
+      Tok->Finalized = true;
+    }
----------------
why do we no longer finalize the child tree?

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:225
+      }
+      Callback.finishRun();
+    }
----------------
I'm sure you got this right, but it's hard for me to evaluate this code because I don't know what `UnwrappedLineConsumer::finishRun()` is for - it's not documented and the implementation is mysterious.

You might consider adding a comment to that interface/method, it's not really related to this change though.

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4198
   LLVM_DEBUG({
-    if (CurrentLines == &Lines)
+    if (CurrentLines == &Lines) {
+      llvm::dbgs() << "Adding unwrapped line:\n";
----------------
nit: you check this condition a bunch of times, and this patch adds more.

giving it a name like `!parsingPPDirective()` would be less cryptic

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4235
+    // FIXME: We format the expanded lines in an extra step that does not give
+    // the formatter all unwrapped lines, thus the indexes are invalid; to allow
+    // all features during expanded line formatting, recalcuate the indexes
----------------
This is a bit vague as to what the "indexes" refer to and what the implications are (between comment & code "indexes" is mentioned 3 times but it's not clear without digging what they are).

AFAICS in practice this only affects indentation of braces in whitesmiths style, which may be worth mentioning. (It'll get stale, but at the moment the comment sounds like arbitrary things are likely to be broken)

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4564
+      // We parse the macro call into a new line.
+      auto Args = parseMacroCall();
+      InExpansion = OldInExpansion;
----------------
it looks like you go into parsing the macro call including parens and arguments regardless of whether the macro is object-like or function-like.

So given
```
#define DEBUG puts
DEBUG("foo");
```
you're going to expand to `puts;` instead of `puts("foo");`

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4573
+      Line = std::move(PreCall);
+      SmallVector<FormatToken *, 8> New = Macros.expand(ID, Args);
+      if (!New.empty())
----------------
Calling a macro with the wrong arity might be worth flagging in debug output?

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4622
+  nextToken();
+  if (FormatTok->isNot(tok::l_paren))
+    return Args;
----------------
this mishandles:

```
#define BAR() foo
int y = BAR; // no macro used here, just a variable named BAR
```

================
Comment at: clang/lib/Format/UnwrappedLineParser.cpp:4660
+    }
+  } while (!eof());
+  return {};
----------------
hitting eof is an error condition here, so it seems more natural to handle it as a case in the switch as a kind of early-return, and maybe it's worth adding an LLVM_DEBUG() for that case. If anything it'd be nice to have r_paren break the loop (but too annoying with the switch inside)

================
Comment at: clang/lib/Format/UnwrappedLineParser.h:78

+  void resetIndexes() {
+    MatchingOpeningBlockLineIndex = kInvalidIndex;
----------------
maybe slightly more descriptive like resetBlockLineIndexes?

in principle you want to clear any indices you have here, so it's nice to be vague, but the caller also needs to reason about the consequences of destroying this data.

================
Comment at: clang/lib/Format/UnwrappedLineParser.h:96
 class FormatTokenSource;
+class MacroCallReconstructor;

----------------
you forward declare this here, move the destructor below out-of-line etc
but also include Macros.h and use MacroExpander by value below
Do we want to avoid the dependency or not?

================
Comment at: clang/lib/Format/UnwrappedLineParser.h:285
+  // until it is finished.
+  std::unique_ptr<MacroCallReconstructor> Reconstruct;
+
----------------
if this doesn't need to be incomplete, optional would be clearer

================
Comment at: clang/unittests/Format/FormatTest.cpp:70
+                     const FormatStyle &Style = getLLVMStyle(),
+                     bool MessUp = true) {
     ScopedTrace t(File, Line, ::testing::Message() << Code.str());
----------------
hmm, testing formatting in this way without messing up the formatting first isn't very convincing from a black-box perspective!

if messUp isn't feasible here (why precisely?) then maybe these tests should specify both input + expected output?

================
Comment at: clang/unittests/Format/FormatTest.cpp:22584
+  Style.Macros.push_back("CALL(x)=f([] { x })");
+  Style.Macros.push_back("ASSIGN_OR_RETURN(a, b, c)=a = (b) || (c)");
+
----------------
If this is assign_or_return, the treatment of "c" is fairly strange. Also you are mostly calling this with only two args. Maybe drop C and use a different macro for the 3-arg case?

================
Comment at: clang/unittests/Format/FormatTest.cpp:22612
+
+  verifyFormat("ASSIGN_OR_RETURN(MySomewhatLongType *variable,\n"
+               "                 MySomewhatLongFunction(SomethingElse()));\n",
----------------
MOCK_METHOD is another motivating case that might be worth testing (even if it's just a more complicated version of the same thing)

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144170/new/

https://reviews.llvm.org/D144170