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

Tue Apr 20 13:45:19 PDT 2021

Kai added inline comments.

================
Comment at: llvm/lib/Support/CharSet.cpp:190
+  }
+#ifdef HAVE_ICONV
+  iconv_t ConvDesc = iconv_open(CPTo, CPFrom);
----------------
ctetreau wrote:
> 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?
This can be viewed as the same as compiling a file: it fails on my machine if I do not have the same file in the same place as my coworker. This problem seems acceptable for the clang -fexec-charset patch.

iconv is a POSIX specification ([[ https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/iconv.h.html | POSIX iconv.h ]]), so there is no license problem. 

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

https://reviews.llvm.org/D88741