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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
