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

Thu Dec 8 15:10:40 PST 2022

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.

================
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 && Diagnose)
----------------
I'm having a hard time understanding what this comment is trying to convey. The comment response to Aaron was much more helpful to me. How about this suggested edit?

================
Comment at: clang/lib/Lex/Lexer.cpp:3386
+
+  if (!Result || CurPtr - StartPtr == (ptrdiff_t)(Buffer.size() + 4))
     StartPtr = CurPtr;
----------------
This is a bit of a tangent, but under what circumstances would `CurPtr - StartPtr` not be equal to `Buffer.size() + 4`? Actually, I'm not sure that +4 is correct. It looks like `StartPtr` is expected to point to `N` at the beginning of the function, so the initial `\` is not included in the range. If `N` isn't seen, the function returns early. Likewise, if either of the `{` or `}` delimiters is not found, the function returns early.

I think the goal of this code is to skip the `getAndAdvanceChar()` machinery when the buffer is being claimed (no need to parse UCNs or trigraphs in the character name), but it looks like, particularly with this DR, that we might always be able to do that.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138861