[PATCH] D93031: Enable fexec-charset option

[Abhina Sree via Phabricator via llvm-commits]

abhina.sreeskantharajan marked 6 inline comments as done.
abhina.sreeskantharajan added inline comments.


================
Comment at: clang/include/clang/Lex/LiteralSupport.h:191
+                    Preprocessor &PP, tok::TokenKind kind,
+                    ConversionState TranslationState = TranslateToExecCharset);
 
----------------
tahonermann wrote:
> Is the default argument for `TranslationState` actually used anywhere?  I'm skeptical that a default argument provides a benefit here.
> 
> Actually, this diff doesn't include any changes to construct a `CharLiteralParser` with an explicit argument.  It seems this argument isn't actually needed.
> 
> The only places I see objects of `CharLiteralParser` type constructed are in `EvaluateValue()` in `clang/lib/Lex/PPExpressions.cpp` and `Sema::ActOnCharacterConstant()` in `clang/lib/Sema/SemaExpr.cpp`.
You're right, we don't have any cases that use this arg yet so we can remove it.


================
Comment at: clang/include/clang/Lex/LiteralSupport.h:238-239
+        ResultPtr(ResultBuf.data()), hadError(false), Pascal(false) {
+    LT = new LiteralTranslator();
+    LT->setTranslationTables(Features, Target, *Diags);
+    init(StringToks, TranslationState);
----------------
tahonermann wrote:
> I don't think a `LiteralTranslator` object is actually needed in this case.  The only use of this constructor that I see is in `ModuleMapParser::consumeToken()` in `clang/lib/Lex/ModuleMap.cpp` and, in that case, I don't think any translation is necessary.
> 
> This suggests that `TranslationState` is not needed for this constructor either; `NoTranslation` can be passed to `init()`.
Thanks, I've removed it.


================
Comment at: clang/include/clang/Lex/Preprocessor.h:145
   ModuleLoader      &TheModuleLoader;
+  LiteralTranslator *LT = nullptr;
 
----------------
tahonermann wrote:
> I don't see a reason for `LT` to be a pointer.  Can it be made a reference or, better, a non-reference, non-pointer data member?
Thanks, I've changed it to a non-reference non-pointer member.


================
Comment at: clang/lib/Lex/LiteralSupport.cpp:1322-1324
+  ConversionState State = isUTFLiteral(Kind) ? NoTranslation : TranslationState;
+  llvm::CharSetConverter *Converter =
+      LT ? LT->getCharConversionTable(State) : nullptr;
----------------
tahonermann wrote:
> Per the comment associated with the constructor declaration, I don't think the new constructor parameter is needed; translation to execution character set is always desired for non-UTF character literals.  I think this can be something like:
>   llvm::CharSetConverter *Converter = nullptr;
>   if (! isUTFLiteral(Kind)) {
>     assert(LT);
>     Converter = LT->getCharConversionTable(TranslateToExecCharset);
>   }
I can't add an assertion here because LT might not be created in the case of the second StringLiteralParser constructor which does not pass the Preprocessor. But I have added the remaining changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93031