sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land.
================ Comment at: clang/lib/Lex/TokenLexer.cpp:1029 + // Including Limit will not mischeck for across-file-id tokens + // because SourceManager allocates FileSize + 1 for each SLocEntry. + // ---------------- Naively, it seems like if recovery tokens could lead us to read one-past-the-end, it could also cause us to read two-past-the-end in which case this check would fail. We're relying on this never happening, so I think the comment should refer more specifically to the tokens that get inserted. Maybe "recovery only ever inserts a single token past the end of the FileID, specifically the `)` when a macro-arg containing a comma should be guarded by parentheses"? ================ Comment at: clang/lib/Lex/TokenLexer.cpp:1031 + // + // See https://github.com/llvm/llvm-project/issues/60722. + return T.getLocation() >= BeginLoc && T.getLocation() <= Limit ---------------- hokein wrote: > shafik wrote: > > I don't believe we include links to issues in comments but you should > > definitely add the information to the commit message and when folks look at > > the commit they can get the that context there. > I'm happy to remove it. But is there any guideline discouraging this > practise? I have already seen this pattern in LLVM project. I think this is > based on the author's judgement (IMO, it seems better to keep it for this > case). I see 50+ refs to github issues, 70+ links to bugs.llvm.org, 1K+ references to PRNNNNN. The reference seems useful when it's a strange workaround like this. ================ Comment at: clang/test/Lexer/update_consecutive_macro_crash.cpp:8 +void foo() { + X(int{,}); // expected-error {{too many arguments provided to function-like macro invocation}} \ + expected-error {{expected expression}} \ ---------------- hokein wrote: > More details about the issue : > > - due to the error recovery, the clang lexer inserts a pair of `()` around > the macro argument `int{,}`, so we will see [`(`, `int`, `{`, `,`, `}`, `)`] > tokens > - however, the size of file id for the macro argument only take account the > written tokens which are [`int`, `{`, `,`, `}`], and the extra inserted `)` > token is at the `Limit` source location which triggers an empty `Partition`. can you add these as comments to the test? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144054/new/ https://reviews.llvm.org/D144054 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
