[PATCH] D93031: Enable fexec-charset option

[Abhina Sree via Phabricator via cfe-commits]

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:
> I don't know the LLVM style guides well, but I suspect a class with all public members should be defined using `struct` and not include access specifiers.
I've made these private.


================
Comment at: clang/include/clang/Lex/LiteralTranslator.h:37
+
+  llvm::CharSetConverter *getConversionTable(const char *Codepage);
+  CharsetTableStatusCode findOrCreateExecCharsetTable(const char *To);
----------------
tahonermann wrote:
> `getConversionTable()` is logically `const`.  Perhaps `ExecCharsetTables` should be `mutable`.
> 
> From a terminology stand point, this function is misnamed.  It doesn't return a table, it returns a converter for an encoding.  I suggest:
>   llvm::CharSetConverter *getCharSetConverter(const char *Encoding) const;
I've renamed this function to getConverter


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93031