[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann added a comment. This has already been merged, but I'm commenting just to note (continued) approval of the most recent changes. Looks great, Corentin! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

This revision was automatically updated to reflect the committed changes. Closed by commit rGdbfe446ef3b2: [Clang] Implement CWG2640 Allow more characters in an n-char sequence (authored by cor3ntin). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added a comment. https://reviews.llvm.org/D139889 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 ___ cfe-commits mailing list

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added a comment. In D138861#3988954 , @tahonermann wrote: > @cor3ntin, I suspect the answer is no, but do your changes perhaps address > this assertion failure? https://godbolt.org/z/1bPcPcWzv > > int \u1{234}; Interesting bug. No, it

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 482279. cor3ntin added a comment. Fix caret position Fix tests (defining a macro named 'a' caused issues) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files:

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann added a comment. @cor3ntin, I suspect the answer is no, but do your changes perhaps address this assertion failure? https://godbolt.org/z/1bPcPcWzv int \u1{234}; Stack dump: clang++: /root/llvm-project/clang/lib/Lex/LiteralSupport.cpp:382: void

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann accepted this revision. tahonermann added a comment. This revision is now accepted and ready to land. This looks good. I'm accepting the change despite one remaining issue that was neither introduced nor addressed by this patch; the diagnostic caret will still be in the wrong

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3386 + + if (!Result || CurPtr - StartPtr == (ptrdiff_t)(Buffer.size() + 4)) StartPtr = CurPtr; tahonermann wrote: > cor3ntin wrote: > > tahonermann wrote: > > > cor3ntin wrote: > > > >

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 481767. cor3ntin added a comment. Format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst clang/lib/Lex/Lexer.cpp

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 481766. cor3ntin added a comment. - Rebase - Add/Improve comments - Add more trigrahs tests - Fix crash when a trigraph appears at the end of a named escape sequence and trigraphs are disabled - Fix when getAndAdvanceChar is called - alas there is no way to

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3323 if (Diagnose) Diag(StartPtr, diag::warn_ucn_escape_incomplete); return llvm::None; I was able to confirm that `StartPtr` does point to `N`. See the diagnostic generated

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3386 + + if (!Result || CurPtr - StartPtr == (ptrdiff_t)(Buffer.size() + 4)) StartPtr = CurPtr; cor3ntin wrote: > tahonermann wrote: > > cor3ntin wrote: > > > cor3ntin wrote: > > > >

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3378-3379 - if (LooseMatch) + // If no diagnostic has been emitted yet we do not want to recover here + // to make sure this function will be called again and a diagnostic emitted then. + if (LooseMatch

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3378-3379 - if (LooseMatch) + // If no diagnostic has been emitted yet we do not want to recover here + // to make sure this function will be called again and a diagnostic emitted then. + if

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added a comment. Thanks Tom, I replied to your comments Comment at: clang/lib/Lex/Lexer.cpp:3386 + + if (!Result || CurPtr - StartPtr == (ptrdiff_t)(Buffer.size() + 4)) StartPtr = CurPtr; cor3ntin wrote: > tahonermann wrote: > > This is a bit of

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added a comment. Thanks Tom, I replied to your comments Comment at: clang/lib/Lex/Lexer.cpp:3378-3379 - if (LooseMatch) + // If no diagnostic has been emitted yet we do not want to recover here + // to make sure this function will be called again and a diagnostic

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

tahonermann requested changes to this revision. tahonermann added a comment. This revision now requires changes to proceed. This looks really good. I added a suggested edit for a comment that I had a hard time understanding and noted an area of code that I'm not sure is working as expected.

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added a comment. @tahonermann Do you want to take a look at this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 ___ cfe-commits mailing list

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 480062. cor3ntin added a comment. Add comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst clang/lib/Lex/Lexer.cpp

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

aaron.ballman accepted this revision as: aaron.ballman. aaron.ballman added a comment. This revision is now accepted and ready to land. LGTM with a request for a comment, but please give @tahonermann a chance to look at the review before landing. Comment at:

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3379 - if (LooseMatch) + if (LooseMatch && Diagnose) Res = LooseMatch->CodePoint; aaron.ballman wrote: > Why do we only want to do this if we're diagnosing? The scenario we want to

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478919. cor3ntin added a comment. Restore a macro test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478914. cor3ntin added a comment. Remove extraneous space and braces Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

aaron.ballman added inline comments. Comment at: clang/lib/Lex/Lexer.cpp:3312 unsigned CharSize; + bool Diagnose = Result && !isLexingRawMode(); Spurious whitespace Comment at: clang/lib/Lex/Lexer.cpp:3379 - if (LooseMatch) + if

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin added inline comments. Comment at: clang/test/CXX/drs/dr26xx.cpp:41 +#define a z( +int x = a\N{abc}); // expected-error 2{{'abc' is not a valid Unicode character name}} + aaron.ballman wrote: > Why do we get two errors here? > > Also, can you add this

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478706. cor3ntin added a comment. Avoid duplicating errors in macros. Because of that we cannot alays recover nicely with a loose matching. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478679. cor3ntin added a comment. Slightly improve error recovery Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478673. cor3ntin added a comment. Add test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/docs/ReleaseNotes.rst clang/lib/Lex/Lexer.cpp

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

aaron.ballman added inline comments. Comment at: clang/test/CXX/drs/dr26xx.cpp:41 +#define a z( +int x = a\N{abc}); // expected-error 2{{'abc' is not a valid Unicode character name}} + Why do we get two errors here? Also, can you add this case: ``` int y =

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478631. cor3ntin added a comment. - Add test - Add release notes entry - use isVerticalWhitespace Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files:

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

aaron.ballman added a comment. Don't forget the release note! :-) Comment at: clang/lib/Lex/Lexer.cpp:3338 -if (!isAlphanumeric(C) && C != '_' && C != '-' && C != ' ') +if (C == '\n' || C == '\r') break; `isVerticalWhitespace()`?

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478373. cor3ntin added a comment. Fix DR status (I didn't know the scrip supported versions, neat) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files:

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

erichkeane added a comment. I dont know the lexer well enough to do better than this review, but the code itself seems fine. Comment at: clang/www/cxx_dr_status.html:15651 Allow more characters in an n-char sequence -Unknown +Yes Shouldn't

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin updated this revision to Diff 478362. cor3ntin added a comment. Update DR web page Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138861/new/ https://reviews.llvm.org/D138861 Files: clang/lib/Lex/Lexer.cpp

[PATCH] D138861: [Clang] Implement CWG2640 Allow more characters in an n-char sequence

cor3ntin created this revision. Herald added a project: All. cor3ntin requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D138861 Files: clang/lib/Lex/Lexer.cpp