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