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

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman added a comment.

In D88741#4232746 <https://reviews.llvm.org/D88741#4232746>, @cor3ntin wrote:

> Thanks for working on this.
>
> Before starting a more in depth review on that, I think this is big enough that we want to see an RFC with wider consensus and interest from the community as far as maintaining this.
> I know of https://discourse.llvm.org/t/rfc-adding-a-char-set-converter-to-support-library/56592/2 and https://discourse.llvm.org/t/rfc-adding-a-char-set-converter-to-support-library/56574 , but neither gather a lot of comments.

+1, neither of those RFCs really showed a strong community consensus behind the idea, so I think another RFC would be appropriate. Please be sure to address the items in https://clang.llvm.org/get_involved.html#criteria explicitly in the RFC where appropriate.

> I am of the opinion that we should rely on iconv as much as possible, as i do not think maintaining conversion table will be a good use of our resources, however i think people might have widely different opinions and you will want 
> to make sure there is community buy-in on that point.
>
> We should note that GCC has had good success using iconv, and I don't think the platform-specific availability of encondings has been an issue in practice but it's worth addressing. I know some people mentioned that point previously.
> IMO, Any user confusion in that regard could be addressed by providing a sufficiently high-quality diagnostic in the presence of unsupported encoding.
>
> If we do use iconv though, i would like us to have a better understanding of use cases, The patch currently links `iconv` to all llvm libraries, which might be overkill if the only project using it is Clang, and I wonder how that affects packaging 
> on linux distributions.
>
> Speaking of, maybe the LLVM foundation will be able to provide legal opinions on linking to `iconv` on various platforms. Maybe something @tstellar  would be able to help with. I would not expect particular challenges but we
> should to make sure.

Agreed -- any time we bring in a third-party dependency, we should be going through an explicit license review just to make sure we're in the clear.

> @aaron.ballman  also reminded me that if `iconv` is generally available on posix-ish platforms, it is usually not available on Windows. Building iconv-like facilities on top of win32 APIs may be challenging, but we might want to think about
> ways to allow or facilitate the use of iconv in Windows build. At least i would like us to give some consideration to that point, Indeed, Windows would, logically be the other big motivation to teach Clang about encodings beyond UTF-8.

This is my big concern. iconv isn't installed on Windows by default, so this means we either need to dynamically link against iconv (which means Windows users now have extra dependencies they need to deal with and this can negatively impact downstream projects that ship an iconv version because of versioning differences) or we need to statically link against iconv (which means Windows users now have a significantly larger binary to deal with, and it still can negatively impact downstream projects that also statically link against iconv). That said, I also don't think we want to try to replace iconv with our own set of APIs. This should be explicitly discussed in the new RFC.

> Are there any other supported platforms for which iconv availability would be a concern?
>
> I think all of these points should be addressed in an RFC so that we have a clear plan moving forward.

+1


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

https://reviews.llvm.org/D88741