[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan abandoned this revision. abhina.sreeskantharajan added a comment. I have opened a new patch https://reviews.llvm.org/D153419 and am closing this revision Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93031/new/

[PATCH] D93031: Enable fexec-charset option

barannikov88 added a comment. In D93031#4309546 , @abhina.sreeskantharajan wrote: > In D93031#4308764 , @barannikov88 > wrote: > >> @abhina.sreeskantharajan >> What is the status of this patch? > > Hello, I was

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added a comment. In D93031#4308764 , @barannikov88 wrote: > @abhina.sreeskantharajan > What is the status of this patch? Hello, I was waiting for the CharSetConverter patch to land. Now that this patch has landed

[PATCH] D93031: Enable fexec-charset option

barannikov88 added a comment. @abhina.sreeskantharajan What is the status of this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93031/new/ https://reviews.llvm.org/D93031 ___ cfe-commits mailing

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added a comment. In D93031#2706660 , @cor3ntin wrote: >>> We should use the original source form of the string literal when >>> pretty-printing a `StringLiteral` or `CharacterLiteral`; there are a bunch >>> of UTF-8 assumptions

[PATCH] D93031: Enable fexec-charset option

cor3ntin added a comment. In D93031#2706988 , @joerg wrote: > "Keeping the original spelling around" would assume that the input is not > using a stateful encoding. That seems worse as assumption than giving the > canonical output in UTF-8 and shifting

[PATCH] D93031: Enable fexec-charset option

joerg added a comment. "Keeping the original spelling around" would assume that the input is not using a stateful encoding. That seems worse as assumption than giving the canonical output in UTF-8 and shifting the problem to the user's editor? Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D93031: Enable fexec-charset option

cor3ntin added a comment. >> We should use the original source form of the string literal when >> pretty-printing a `StringLiteral` or `CharacterLiteral`; there are a bunch >> of UTF-8 assumptions baked into `StmtPrinter` that will need revisiting. And >> we'll need to modify the handful of

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 339355. abhina.sreeskantharajan added a reviewer: ThePhD. abhina.sreeskantharajan added a comment. Thanks for catching that. This sets the __clang_literal_encoding__ to Opts.ExecCharset or defaults to SystemCharset. Repository: rG LLVM

[PATCH] D93031: Enable fexec-charset option

ThePhD added a comment. Just a tiny comment: could you please make sure the name of the resolved encoding is also propagated to InitPreprocessor.cpp that sets the `__clang_literal_encoding__` macro? (https://github.com/llvm/llvm-project/blob/main/clang/lib/Frontend/InitPreprocessor.cpp#L784)

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 338565. abhina.sreeskantharajan added a comment. Rebase + set size of char as 1 when creating a StringRef to fix lit failure Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93031/new/

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 336417. abhina.sreeskantharajan added a comment. Accidentally added dependent patch in this one. Removing that Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93031/new/

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 336416. abhina.sreeskantharajan added a comment. Rebase + fix CharLiteralParser endian issue by saving the char to a char variable first and then creating a StringRef Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 3 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/include/clang/Lex/LiteralTranslator.h:30-31 + +class LiteralTranslator { +public: + llvm::StringRef InternalCharset; tahonermann wrote: >

[PATCH] D93031: Enable fexec-charset option

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); + }

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 328513. abhina.sreeskantharajan marked an inline comment as done. abhina.sreeskantharajan added a comment. Add assertion, add testcase for multi-line raw string Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 4 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/include/clang/Lex/Preprocessor.h:145 ModuleLoader + LiteralConverter LT; rsmith wrote: > Please give this a longer name.

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 328151. abhina.sreeskantharajan added a comment. Addressing some more comments. Updating the argument parsing, lit tests, some more renaming. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D93031: Enable fexec-charset option

rsmith added inline comments. Comment at: clang/include/clang/Lex/Preprocessor.h:145 ModuleLoader + LiteralConverter LT; Please give this a longer name. Abbreviation names should only be used in fairly small scopes where it's easy to look up what

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added a comment. Hi Tom, @tahonermann I renamed the LiteralTranslator class to LiteralConverter.cpp and have renamed a lot of the functions. Let me know what you think. I agree that the setConverters function is awkward, the problem stems from initializing the member

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 6 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/include/clang/Lex/LiteralSupport.h:191 +Preprocessor , tok::TokenKind kind, +ConversionState TranslationState =

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 327470. abhina.sreeskantharajan added a comment. Thanks for the feedback! I haven't addressed all the comments yet but I've made major renaming changes and hope to get feedback on it. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D93031: Enable fexec-charset option

jansvoboda11 added inline comments. Comment at: clang/include/clang/Driver/Options.td:3583 +def fexec_charset : Separate<["-"], "fexec-charset">, MetaVarName<"">, + HelpText<"Set the execution for string and character literals. " Could you switch to the

[PATCH] D93031: Enable fexec-charset option

tahonermann added a comment. Hi, Abhina. Sorry for the delay getting back to you. I added some more comments. Comment at: clang/include/clang/Lex/LiteralSupport.h:191 +Preprocessor , tok::TokenKind kind, +ConversionState

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added a comment. Herald added a reviewer: jansvoboda11. ping :) Is there any more feedback on the implementation inside ProcessCharEscape()? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93031/new/

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Lex/LiteralSupport.cpp:1593-1597 + ConversionState State = TranslationState; + if (Kind == tok::wide_string_literal) +State = TranslateToSystemCharset; + else if (isUTFLiteral(Kind)) +State =

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 2 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Lex/LiteralSupport.cpp:235 +Converter->convert(std::string(1, ByteChar), ResultCharConv); +memcpy((void *), ResultCharConv.data(),

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 314115. abhina.sreeskantharajan added a comment. This patch replaces the memcpy in CharLiteralParser with an assignment. I've added an assertion for cases where the character size increases after translation. Repository: rG LLVM Github

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked an inline comment as done. abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Lex/LiteralSupport.cpp:234 +SmallString<8> ResultCharConv; +Converter->convert(std::string(1, ByteChar), ResultCharConv); +memcpy((void *),

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 8 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/include/clang/Lex/LiteralSupport.h:244 bool Pascal; + ConversionState TranslationState; + tahonermann wrote: > abhina.sreeskantharajan

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 313990. abhina.sreeskantharajan added a comment. Thanks for the review! I've addressed most of the comments but I still need to work on the translation issues in CharLiteralParser that was kindly pointed out by Tom and Richard. Here are the

[PATCH] D93031: Enable fexec-charset option

tahonermann added inline comments. Comment at: clang/include/clang/Lex/LiteralSupport.h:244 bool Pascal; + ConversionState TranslationState; + abhina.sreeskantharajan wrote: > tahonermann wrote: > > Same concern here with respect to persisting the conversion

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Lex/LiteralSupport.cpp:1322-1323 + TranslationState = translationState; + if (Kind == tok::wide_string_literal) +TranslationState = TranslateToSystemCharset; + else if (isUTFLiteral(Kind))

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 11 inline comments as done. abhina.sreeskantharajan added inline comments. Comment at: clang/include/clang/Driver/Options.td:3583-3584 +def fexec_charset : Separate<["-"], "fexec-charset">, MetaVarName<"">, + HelpText<"Set the execution for

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 313112. abhina.sreeskantharajan added a comment. Thanks for your patience, I've addressed some more comments. Here is the summary of the changes in this patch: - add translation for UCN strings, update testcase - fix helptext for

[PATCH] D93031: Enable fexec-charset option

tahonermann added inline comments. Comment at: clang/test/CodeGen/systemz-charset.c:4 + +char *UpperCaseLetters = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; +// CHECK: c"\C1\C2\C3\C4\C5\C6\C7\C8\C9\D1\D2\D3\D4\D5\D6\D7\D8\D9\E2\E3\E4\E5\E6\E7\E8\E9\00" `const char *` please

[PATCH] D93031: Enable fexec-charset option

tahonermann added inline comments. Comment at: clang/include/clang/Driver/Options.td:3583-3584 +def fexec_charset : Separate<["-"], "fexec-charset">, MetaVarName<"">, + HelpText<"Set the execution for string and character literals">; def target_cpu : Separate<["-"],

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan marked 9 inline comments as done. abhina.sreeskantharajan added a comment. In D93031#2447230 , @rsmith wrote: > I'm overall pretty happy about how clean and non-invasive the changes > required here are. But please make sure you

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan updated this revision to Diff 311911. abhina.sreeskantharajan added a comment. Thanks for your quick reviews! I haven't addressed all the comments yet but I plan to address all of them. I put up this patch early because it has a few major changes: - moves

[PATCH] D93031: Enable fexec-charset option

tahonermann added inline comments. Comment at: clang/include/clang/Driver/Options.td:3583-3584 +def fexec_charset : Separate<["-"], "fexec-charset">, MetaVarName<"">, + HelpText<"Set the execution for string and character literals">; def target_cpu : Separate<["-"],

[PATCH] D93031: Enable fexec-charset option

rsmith added a comment. I'm overall pretty happy about how clean and non-invasive the changes required here are. But please make sure you don't change the encodings of `u8"..."` / `u"..."` / `U"..."` literals; those need to stay as UTF-8 / UTF-16 / UTF-32. Also, we should have a story for how

[PATCH] D93031: Enable fexec-charset option

abhina.sreeskantharajan created this revision. Herald added subscribers: dexonsmith, dang, hiraditya, mgorny. abhina.sreeskantharajan requested review of this revision. Herald added projects: clang, LLVM. Herald added subscribers: llvm-commits, cfe-commits. This patch enables the fexec-charset