sammccall added inline comments.
================ Comment at: clang/lib/Format/MacroExpander.cpp:186 + Tok->MacroCtx = MacroContext(MR_ExpandedArg); + pushToken(Tok); + } ---------------- 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? 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
