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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
