[PATCH] D88741: [SystemZ/z/OS] Add utility class for char set conversion.

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added inline comments.


================
Comment at: llvm/lib/Support/CharSet.cpp:26
+
+const char *CharSetConverter::CP_IBM1047 = "IBM-1047";
+const char *CharSetConverter::CP_UTF8 = "UTF-8";
----------------
These names leak the iconv dependency, and can be error prone if somebody types them wrong. We should have an enum class for the set of char sets that can be converted. If we keep the iconv dependency, it's trivial to map `CharSet::IBM1047 -> "IBM-1047"`, but the compiler can catch the error if you mistype the enum name.


================
Comment at: llvm/lib/Support/CharSet.cpp:158
+
+ErrorOr<CharSetConverter> CharSetConverter::create(const char *CPFrom,
+                                                   const char *CPTo) {
----------------
If we replace the name strings with enums, then it should be possible to make this function total and remove the `ErrorOr`


================
Comment at: llvm/lib/Support/CharSet.cpp:190
+  }
+#ifdef HAVE_ICONV
+  iconv_t ConvDesc = iconv_open(CPTo, CPFrom);
----------------
If I'm understanding this correctly, having iconv provides the possibility of supporting conversions other than to and from ascii, utf8, and ebcdic? I'm concerned that this is going to create a ton of bug reports of the form "CharSetConverter::create returned an error on my machine, but not my coworker's machine!" which will be closed as "operator error, install iconv". I feel like there should be a set of conversions supported by CharSetConverter, and they should work regardless of the presense of iconv.

>From messages I've seen on the mailing lists, it sounds like there is license uncertainty with linking iconv. Maybe it's best to just not have this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88741