add error checking for text_encoding function, address other comments

Author: abhina-sree

llvm/cmake/config-ix.cmake

@@ -295,13 +295,13 @@ if(LLVM_HAS_LOGF128)
 endif()
 
 if (LLVM_ENABLE_ICU STREQUAL FORCE_ON AND LLVM_ENABLE_ICONV STREQUAL FORCE_ON)
-  message(FATAL_ERROR "Both LLVM_ENABLE_ICU and LLVM_ENABLE_ICONV should not be FORCE_ON")
+  message(FATAL_ERROR "LLVM_ENABLE_ICU and LLVM_ENABLE_ICONV should not both be FORCE_ON")
 endif()
 
-# Check for ICU.
+# Check for ICU. Only allow an optional, dynamic link for ICU so we don't impact LLVM's licensing.
 if(LLVM_ENABLE_ICU AND NOT(LLVM_ENABLE_ICONV STREQUAL FORCE_ON))
   set(LIBRARY_SUFFIXES ${CMAKE_FIND_LIBRARY_SUFFIXES})
-  set(CMAKE_FIND_LIBRARY_SUFFIXES ".so")
+  set(CMAKE_FIND_LIBRARY_SUFFIXES "${CMAKE_SHARED_LIBRARY_SUFFIX}")
   if (LLVM_ENABLE_ICU STREQUAL FORCE_ON)
     find_package(ICU REQUIRED COMPONENTS uc i18n)
     if (NOT ICU_FOUND)

llvm/include/llvm/Support/CharSet.h

@@ -20,7 +20,6 @@
 #include "llvm/Config/config.h"
 #include "llvm/Support/ErrorOr.h"
 
-#include <functional>
 #include <string>
 #include <system_error>
 
@@ -89,11 +88,13 @@ class CharSetConverter {
 
 public:
   /// Creates a CharSetConverter instance.
+  /// Returns std::errc::invalid_argument in case the requested conversion is
+  /// not supported.
   /// \param[in] CSFrom the source character encoding
   /// \param[in] CSTo the target character encoding
-  /// \return a CharSetConverter instance
-  static CharSetConverter create(text_encoding::id CSFrom,
-                                 text_encoding::id CSTo);
+  /// \return a CharSetConverter instance or an error code
+  static ErrorOr<CharSetConverter> create(text_encoding::id CSFrom,
+                                          text_encoding::id CSTo);
 
   /// Creates a CharSetConverter instance.
   /// Returns std::errc::invalid_argument in case the requested conversion is

llvm/lib/Support/CharSet.cpp

@@ -57,10 +57,9 @@ static std::optional<text_encoding::id> getKnownCharSet(StringRef CSName) {
   return std::nullopt;
 }
 
-#if defined(HAVE_ICONV) || defined(HAVE_ICU)
-static void HandleOverflow(size_t &Capacity, char *&Output,
-                           size_t &OutputLength,
-                           SmallVectorImpl<char> &Result) {
+LLVM_ATTRIBUTE_UNUSED static void
+HandleOverflow(size_t &Capacity, char *&Output, size_t &OutputLength,
+               SmallVectorImpl<char> &Result) {
   // No space left in output buffer. Double the size of the underlying
   // memory in the SmallVectorImpl, adjust pointer and length and continue
   // the conversion.
@@ -72,7 +71,6 @@ static void HandleOverflow(size_t &Capacity, char *&Output,
   Output = static_cast<char *>(Result.data());
   OutputLength = Capacity;
 }
-#endif
 
 namespace {
 enum ConversionType {
@@ -290,8 +288,8 @@ void CharSetConverterIconv::reset() {
 #endif // HAVE_ICONV
 } // namespace
 
-CharSetConverter CharSetConverter::create(text_encoding::id CPFrom,
-                                          text_encoding::id CPTo) {
+ErrorOr<CharSetConverter> CharSetConverter::create(text_encoding::id CPFrom,
+                                                   text_encoding::id CPTo) {
 
   assert(CPFrom != CPTo && "Text encodings should be distinct");
 
@@ -302,19 +300,22 @@ CharSetConverter CharSetConverter::create(text_encoding::id CPFrom,
            CPTo == text_encoding::id::UTF8)
     Conversion = IBM1047ToUTF8;
   else
-    llvm_unreachable("Invalid ConversionType!");
+    return std::error_code(errno, std::generic_category());
+
   std::unique_ptr<details::CharSetConverterImplBase> Converter =
       std::make_unique<CharSetConverterTable>(Conversion);
-
   return CharSetConverter(std::move(Converter));
 }
 
 ErrorOr<CharSetConverter> CharSetConverter::create(StringRef CSFrom,
                                                    StringRef CSTo) {
   std::optional<text_encoding::id> From = getKnownCharSet(CSFrom);
   std::optional<text_encoding::id> To = getKnownCharSet(CSTo);
-  if (From && To)
-    return create(*From, *To);
+  if (From && To) {
+    ErrorOr<CharSetConverter> Converter = create(*From, *To);
+    if (Converter)
+      return Converter;
+  }
 #ifdef HAVE_ICU
   UErrorCode EC = U_ZERO_ERROR;
   UConverterUniquePtr FromConvDesc(ucnv_open(CSFrom.str().c_str(), &EC));

llvm/unittests/Support/CharSetTest.cpp

@@ -58,30 +58,38 @@ TEST(CharSet, FromUTF8) {
   StringRef Src(HelloA);
   SmallString<64> Dst;
 
-  CharSetConverter Conv = CharSetConverter::create(text_encoding::id::UTF8,
-                                                   text_encoding::id::IBM1047);
-  std::error_code EC = Conv.convert(Src, Dst);
+  ErrorOr<CharSetConverter> Conv = CharSetConverter::create(
+      text_encoding::id::UTF8, text_encoding::id::IBM1047);
+
+  // Stop test if conversion is not supported.
+  if (!Conv) {
+    ASSERT_EQ(Conv.getError(),
+              std::make_error_code(std::errc::invalid_argument));
+    return;
+  }
+
+  std::error_code EC = Conv->convert(Src, Dst);
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(HelloE, static_cast<std::string>(Dst).c_str());
   Dst.clear();
 
   // ABC string.
   Src = ABCStrA;
-  EC = Conv.convert(Src, Dst);
+  EC = Conv->convert(Src, Dst);
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(ABCStrE, static_cast<std::string>(Dst).c_str());
   Dst.clear();
 
   // Accent string.
   Src = AccentUTF;
-  EC = Conv.convert(Src, Dst);
+  EC = Conv->convert(Src, Dst);
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(AccentE, static_cast<std::string>(Dst).c_str());
   Dst.clear();
 
   // Cyrillic string. Results in error because not representable in 1047.
   Src = CyrillicUTF;
-  EC = Conv.convert(Src, Dst);
+  EC = Conv->convert(Src, Dst);
   EXPECT_EQ(EC, std::errc::illegal_byte_sequence);
 }
 
@@ -90,23 +98,32 @@ TEST(CharSet, ToUTF8) {
   StringRef Src(HelloE);
   SmallString<64> Dst;
 
-  CharSetConverter Conv = CharSetConverter::create(text_encoding::id::IBM1047,
-                                                   text_encoding::id::UTF8);
-  std::error_code EC = Conv.convert(Src, Dst);
+  ErrorOr<CharSetConverter> Conv = CharSetConverter::create(
+      text_encoding::id::IBM1047, text_encoding::id::UTF8);
+
+  // Stop test if conversion is not supported.
+  if (!Conv) {
+    ASSERT_EQ(Conv.getError(),
+              std::make_error_code(std::errc::invalid_argument));
+    return;
+  }
+
+  std::error_code EC = Conv->convert(Src, Dst);
+
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(HelloA, static_cast<std::string>(Dst).c_str());
   Dst.clear();
 
   // ABC string.
   Src = ABCStrE;
-  EC = Conv.convert(Src, Dst);
+  EC = Conv->convert(Src, Dst);
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(ABCStrA, static_cast<std::string>(Dst).c_str());
   Dst.clear();
 
   // Accent string.
   Src = AccentE;
-  EC = Conv.convert(Src, Dst);
+  EC = Conv->convert(Src, Dst);
   EXPECT_TRUE(!EC);
   EXPECT_STREQ(AccentUTF, static_cast<std::string>(Dst).c_str());
 }