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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added inline comments.


================
Comment at: clang/lib/Format/MacroExpander.cpp:186
+        Tok->MacroCtx = MacroContext(MR_ExpandedArg);
+      pushToken(Tok);
+    }
----------------
sammccall wrote:
> 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)
Ugh. I'll need to take a deeper look, but generally, the problem is we don't want to copy - we're mutating the data of the token while formatting the expanded token stream, and then re-use that info when formatting the original stream.
We could copy, add a reference to the original token, and then have a step that adapts in the end, and perhaps that's cleaner overall anyway, but will be quite a change.
The alternative is that I'll look into how to specifically handle double-expansion (or ... forbid it).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83296