[PATCH] D104975: Implement P1949

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added inline comments.


================
Comment at: clang/lib/Lex/Lexer.cpp:1444
+    static const llvm::sys::UnicodeCharSet XIDContinueChars(XIDContinueRanges);
+    return C == '_' || XIDStartChars.contains(C) ||
+           XIDContinueChars.contains(C);
----------------
aaron.ballman wrote:
> Is looking at start chars correct? I was thinking this should only look at the continuation characters because `isAllowedInitiallyIDChar` handles the start of an identifier.
Yes,  But Maybe it needs a comment.
Per Unicode spec, XIDContinueChars is a superset of XIDStartChars, and in the code, we don't deduplicate the table to not make them too big.
So we look at both tables, starting with start as it's the most likely to contain the character 


================
Comment at: clang/test/CXX/drs/dr2xx.cpp:600
 
-namespace dr248 { // dr248: yes c++11
-  // FIXME: Should this also apply to c++98 mode? This was a DR against C++98.
----------------
aaron.ballman wrote:
> This means we're going to drop our support of this DR on https://clang.llvm.org/cxx_dr_status.html when that page gets regenerated. What should our status of that DR be with these changes?
Good question!
I will change it to `Superseded by P1949`. What do you think?


================
Comment at: clang/test/Lexer/unicode.c:40-43
+extern int 👷‍♀; // expected-warning {{declaration does not declare anything}}
+//expected-error at -1 {{character <U+1F477> not allowed in identifier}}
+//expected-error at -2 {{character <U+200D> not allowed in identifier}}
+//expected-error at -3 {{character <U+2640> not allowed in identifier}}
----------------
aaron.ballman wrote:
> Then we don't have to do line number math to see what line the diagnostics are attached to. :-)
It makes me feel dirty but I'll change it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104975/new/

https://reviews.llvm.org/D104975