[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan added a comment. In D153418#4478766 , @tahonermann wrote: >> Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think >> adding support for ICU would be the better long-term solution since it seems >> to

[PATCH] D153418: Adding iconv support to CharSetConverter class

cor3ntin added a comment. In D153418#4478766 , @tahonermann wrote: >> Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think >> adding support for ICU would be the better long-term solution since it seems >> to allow the same

[PATCH] D153418: Adding iconv support to CharSetConverter class

tahonermann added a subscriber: hubert.reinterpretcast. tahonermann added a comment. > Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think > adding support for ICU would be the better long-term solution since it seems > to allow the same behaviour across different

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. > I think this is reasonable since gcc's fexec-charset option also says the > name can be any encoding supported by the iconv library (copy pasted below > from https://gcc.gnu.org/onlinedocs/gcc-13.1.0/gcc.pdf) so this would match > that behaviour "gcc did it"

[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan added a comment. In D153418#4441603 , @efriedma wrote: > Even if we do decide we have to use platform-specific facilities because > there's no suitable library, I think we should at least have a hardcoded set > of encodings we

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. Even if we do decide we have to use platform-specific facilities because there's no suitable library, I think we should at least have a hardcoded set of encodings we recognize, so we aren't passing arbitrary encoding names directly from the command-line to the iconv()

[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan marked an inline comment as done. abhina.sreeskantharajan added a comment. Please correct me if I'm wrong, I'm not too familiar with icu4c, but I think adding support for ICU would be the better long-term solution since it seems to allow the same behaviour across

[PATCH] D153418: Adding iconv support to CharSetConverter class

cor3ntin added a comment. In D153418#4438766 , @efriedma wrote: > This doesn't really address the concerns from > https://discourse.llvm.org/t/rfc-adding-a-charset-converter-to-the-llvm-support-library/69795/17 > about consistency. It's bad if

[PATCH] D153418: Adding iconv support to CharSetConverter class

efriedma added a comment. This doesn't really address the concerns from https://discourse.llvm.org/t/rfc-adding-a-charset-converter-to-the-llvm-support-library/69795/17 about consistency. It's bad if different hosts support a different set of charsets/charset names, and it's really bad if a

[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan marked an inline comment as done. abhina.sreeskantharajan added inline comments. Comment at: clang/lib/Basic/CMakeLists.txt:62 +if(Iconv_FOUND AND NOT Iconv_IS_BUILT_IN) + set(system_libs ${system_libs} ${Iconv_LIBRARIES}) +endif()

[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan updated this revision to Diff 533352. abhina.sreeskantharajan added a reviewer: michaelplatings. abhina.sreeskantharajan added a comment. Use target_link_libraries instead Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D153418: Adding iconv support to CharSetConverter class

michaelplatings added inline comments. Comment at: clang/lib/Basic/CMakeLists.txt:62 +if(Iconv_FOUND AND NOT Iconv_IS_BUILT_IN) + set(system_libs ${system_libs} ${Iconv_LIBRARIES}) +endif() This doesn't look like an idomatic way to link a library. Could you use

[PATCH] D153418: Adding iconv support to CharSetConverter class

abhina.sreeskantharajan created this revision. Herald added a project: All. abhina.sreeskantharajan requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This patch adds iconv support to the CharSetConverter class. Repository: rG LLVM Github