[PATCH] D93031: Enable fexec-charset option

[Abhina Sree via Phabricator via llvm-commits]

abhina.sreeskantharajan added inline comments.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1367
+            Converter->convert(StringRef((char *)tmp_out_start), ConvertedChar);
+            memmove((void *)tmp_out_start, ConvertedChar.data(), 1);
+          }
----------------
rsmith wrote:
> What assurance do we have that 1 output character is correct? I would expect we need to reject with a diagnostic if the character doesn't fit in one converted character.
Right, I'll add a similar assertion to the one we have above.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1700-1702
         // Point into the \n inside the \r\n sequence and operate on the
         // remaining portion of the literal.
         RemainingTokenSpan = AfterCRLF.substr(1);
----------------
rsmith wrote:
> Do we need to convert the newline character too?
> 
> Perhaps for raw string literals it'd be better to do the normal processing here and then convert the entire string at once?
Yes, we need to convert newlines as well. I think the current behaviour is already converting multi line raw strings correctly. I'll add a testcase for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93031