[PATCH] D83296: [clang-format] Add a MacroExpander.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Somehow I missed the email from your replies.

Mostly nits that you can take or leave, but I think potential bugs around functionlike-vs-objectlike and multiple-expansion of args.



================
Comment at: clang/lib/Format/FormatToken.h:177
+/// EndOfExpansion:   0  0 0       2    1 0 1
+struct MacroContext {
+  MacroContext(MacroRole Role) : Role(Role) {}
----------------
"context" is often pretty vague - "MacroSource" isn't a brilliant name but at least seems to hint at the direction (that the FormatToken is the expanded token and the MacroSource gives information about what it was expanded from)

I don't feel strongly about this though, up to you.


================
Comment at: clang/lib/Format/FormatToken.h:646
 private:
-  // Disallow copying.
+  // Only allow copying via the explicit copyInto method.
   FormatToken(const FormatToken &) = delete;
----------------
nit: comment -> copyFrom


================
Comment at: clang/lib/Format/MacroExpander.cpp:1
+//===--- MacroExpander.h - Format C++ code ----------------------*- C++ -*-===//
+//
----------------
nit: banner is for wrong filename


================
Comment at: clang/lib/Format/MacroExpander.cpp:81
+      nextToken();
+    }
+    if (Current->isNot(tok::r_paren))
----------------
this accepts `FOO(A,B,)=...` as equivalent to `FOO(A,B)=...`. Not sure if worth fixing.


================
Comment at: clang/lib/Format/MacroExpander.cpp:88
+
+  bool parseExpansion() {
+    if (!Current->isOneOf(tok::equal, tok::eof))
----------------
(nit: I'd probably find this easier to follow as `if (equal) else if (eof) else` with parseTail inlined, but up to you)


================
Comment at: clang/lib/Format/MacroExpander.cpp:131
+void MacroExpander::parseDefinitions(const std::vector<std::string> &Macros) {
+  for (const std::string &Macro : Macros) {
+    Buffers.push_back(
----------------
uber-nit: seems like this loop belongs in the caller


================
Comment at: clang/lib/Format/MacroExpander.cpp:142
+      auto Definition = Parser.parse();
+      Definitions[Definition.Name] = Definition;
+    }
----------------
nit: this is a copy for what seems like no reason - move `Parser.parse()` inline to this line?


================
Comment at: clang/lib/Format/MacroExpander.cpp:155
+  SmallVector<FormatToken *, 8> Result;
+  const Definition &Def = Definitions.lookup(ID->TokenText);
+
----------------
lookup() returns a value, so this is a copy (with lifetime-extension)
I think you want `*find`


================
Comment at: clang/lib/Format/MacroExpander.cpp:178
+      return true;
+    for (const auto &Tok : Args[I->getValue()]) {
+      // A token can be part of multiple macro arguments.
----------------
please use a different name for this variable, or the parameter it shadows, or preferably both!


================
Comment at: clang/lib/Format/MacroExpander.cpp:179
+    for (const auto &Tok : Args[I->getValue()]) {
+      // A token can be part of multiple macro arguments.
+      // For example, with "ID(x) x":
----------------
nit: "part of a macro argument at multiple levels"?
(Current text suggests to me that it can be arg 0 and arg 1 of the same macro)


================
Comment at: clang/lib/Format/MacroExpander.cpp:186
+        Tok->MacroCtx = MacroContext(MR_ExpandedArg);
+      pushToken(Tok);
+    }
----------------
you're pushing here without copying. This means the original tokens from the ArgsList are mutated. Maybe we own them, but this seems at least wrong for multiple expansion of the same arg.

e.g.
```
#define M(X,Y) X Y X
M(1,2)
```

Will expand to:
- 1, ExpandedArg, ExpandedFrom = [M, M] // should just be one M
- 2, ExpandedArg, ExpandedFrom = [M]
- 1, ExpandedArg, ExpandedFrom = [M, M] // this is the same token pointer as the first one

Maybe it would be better if pushToken performed the copy, and returned a mutable pointer to the copy. (If you can make the input const, that would prevent this type of bug)


================
Comment at: clang/lib/Format/Macros.h:82
+///
+/// Furthermore, expansion and definition are independent - a macro defined
+/// as "A()=a" and "A=a" can both be expanded  as "A()" and "A".
----------------
Is this saying that the functionlike vs objectlike distiction is not preserved?

This doesn't seem safe (unless the *caller* is required to retain this information).
e.g. 
```
#define NUMBER int
using Calculator = NUMBER(); // does expansion consume () or not?
```


================
Comment at: clang/lib/Format/Macros.h:86
+public:
+  using ArgsList = llvm::ArrayRef<llvm::SmallVector<FormatToken *, 8>>;
+
----------------
(Seems a little odd that these pointers to external FormatTokens aren't const... I can believe there's a reason though)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83296