[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:
> klimek wrote:
> > 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).
> > (or ... forbid it).
> 
> I'm starting to think this is the best option.
> 
> The downsides seem pretty acceptable to me:
>  - it's another wart to document: on the other hand it simplifies the conceptual model, I think it helps users understand the deeper behavior
>  - some macros require simplification rather than supplying the actual definition: already crossed this bridge by not supporting macros in macro bodies, variadics, pasting...
>  - loses information: one expansion is enough to establish which part of the grammar the arguments form in realistic cases. (Even in pathological cases, preserving the conflicting info only helps you if you have a plan to resolve the conflicts)
>  - it's another wart to document: 
> 
> Are there any others?
> 
My main concern is that it's probably the most surprising feature to not support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83296