address some comments

Author: abhina-sree

llvm/cmake/config-ix.cmake

@@ -310,7 +310,7 @@ if(LLVM_ENABLE_ICU)
   set(CMAKE_FIND_LIBRARY_SUFFIXES ${LIBRARY_SUFFIXES})
 endif()
 
-# Check for iconv.
+# Check for builtin iconv to avoid licensing issues.
 if(LLVM_ENABLE_ICONV)
   if (LLVM_ENABLE_ICONV STREQUAL FORCE_ON)
     find_package(Iconv REQUIRED)

llvm/include/llvm/Support/CharSet.h

@@ -33,6 +33,9 @@ class CharSetConverterImplBase {
 public:
   virtual ~CharSetConverterImplBase() = default;
 
+  /// Resets the converter to the initial state.
+  virtual void reset() = 0;
+
   /// Converts a string.
   /// \param[in] Source source string
   /// \param[out] Result container for converted string
@@ -52,8 +55,12 @@ class CharSetConverterImplBase {
   /// In case of an error, the result string contains the successfully converted
   /// part of the input string.
   ///
-  virtual std::error_code convert(StringRef Source,
-                                  SmallVectorImpl<char> &Result) const = 0;
+
+  std::error_code convert(StringRef Source,
+                          SmallVectorImpl<char> &Result) const;
+
+  virtual std::error_code convertString(StringRef Source,
+                                        SmallVectorImpl<char> &Result) = 0;
 };
 } // namespace details
 
@@ -111,12 +118,15 @@ class CharSetConverter {
   /// \return error code in case something went wrong
   std::error_code convert(StringRef Source,
                           SmallVectorImpl<char> &Result) const {
-    return Converter->convert(Source, Result);
+    auto EC = Converter->convertString(Source, Result);
+    Converter->reset();
+    return EC;
   }
 
   ErrorOr<std::string> convert(StringRef Source) const {
     SmallString<100> Result;
-    auto EC = Converter->convert(Source, Result);
+    auto EC = Converter->convertString(Source, Result);
+    Converter->reset();
     if (!EC)
       return std::string(Result);
     return EC;

llvm/lib/Support/CharSet.cpp

@@ -91,13 +91,15 @@ class CharSetConverterTable : public details::CharSetConverterImplBase {
 public:
   CharSetConverterTable(ConversionType ConvType) : ConvType(ConvType) {}
 
-  std::error_code convert(StringRef Source,
-                          SmallVectorImpl<char> &Result) const override;
+  std::error_code convertString(StringRef Source,
+                                SmallVectorImpl<char> &Result) override;
+
+  void reset() override {}
 };
 
 std::error_code
-CharSetConverterTable::convert(StringRef Source,
-                               SmallVectorImpl<char> &Result) const {
+CharSetConverterTable::convertString(StringRef Source,
+                                     SmallVectorImpl<char> &Result) {
   if (ConvType == IBM1047ToUTF8) {
     ConverterEBCDIC::convertToUTF8(Source, Result);
     return std::error_code();
@@ -127,13 +129,15 @@ class CharSetConverterICU : public details::CharSetConverterImplBase {
       : FromConvDesc(std::move(FromConverter)),
         ToConvDesc(std::move(ToConverter)) {}
 
-  std::error_code convert(StringRef Source,
-                          SmallVectorImpl<char> &Result) const override;
+  std::error_code convertString(StringRef Source,
+                                SmallVectorImpl<char> &Result) override;
+
+  void reset() override;
 };
 
 std::error_code
-CharSetConverterICU::convert(StringRef Source,
-                             SmallVectorImpl<char> &Result) const {
+CharSetConverterICU::convertString(StringRef Source,
+                                   SmallVectorImpl<char> &Result) {
   // Setup the input in case it has no backing data.
   size_t InputLength = Source.size();
   const char *In = InputLength ? const_cast<char *>(Source.data()) : "";
@@ -171,6 +175,11 @@ CharSetConverterICU::convert(StringRef Source,
   return std::error_code();
 }
 
+void CharSetConverterICU::reset() {
+  ucnv_reset(&*FromConvDesc);
+  ucnv_reset(&*ToConvDesc);
+}
+
 #elif defined(HAVE_ICONV)
 class CharSetConverterIconv : public details::CharSetConverterImplBase {
   class UniqueIconvT {
@@ -202,13 +211,15 @@ class CharSetConverterIconv : public details::CharSetConverterImplBase {
   CharSetConverterIconv(UniqueIconvT ConvDesc)
       : ConvDesc(std::move(ConvDesc)) {}
 
-  std::error_code convert(StringRef Source,
-                          SmallVectorImpl<char> &Result) const override;
+  std::error_code convertString(StringRef Source,
+                                SmallVectorImpl<char> &Result) override;
+
+  void reset() override;
 };
 
 std::error_code
-CharSetConverterIconv::convert(StringRef Source,
-                               SmallVectorImpl<char> &Result) const {
+CharSetConverterIconv::convertString(StringRef Source,
+                                     SmallVectorImpl<char> &Result) {
   // Setup the output. We directly write into the SmallVector.
   size_t Capacity = Result.capacity();
   char *Output = static_cast<char *>(Result.data());
@@ -262,10 +273,14 @@ CharSetConverterIconv::convert(StringRef Source,
   } while (true);
 
   // Re-adjust size to actual size.
-  Result.resize(Capacity - OutputLength);
+  Result.resize(Output - Result.data());
   return std::error_code();
 }
 
+void CharSetConverterIconv::reset() {
+  iconv(ConvDesc, nullptr, nullptr, nullptr, nullptr);
+}
+
 #endif // HAVE_ICONV
 } // namespace
 
@@ -281,8 +296,7 @@ CharSetConverter CharSetConverter::create(text_encoding::id CPFrom,
            CPTo == text_encoding::id::UTF8)
     Conversion = IBM1047ToUTF8;
   else
-    assert(false &&
-           "Only conversions between UTF-8 and IBM-1047 are supported");
+    llvm_unreachable("Invalid ConversionType!");
   std::unique_ptr<details::CharSetConverterImplBase> Converter =
       std::make_unique<CharSetConverterTable>(Conversion);
 
@@ -316,6 +330,7 @@ ErrorOr<CharSetConverter> CharSetConverter::create(StringRef CSFrom,
   std::unique_ptr<details::CharSetConverterImplBase> Converter =
       std::make_unique<CharSetConverterIconv>(ConvDesc);
   return CharSetConverter(std::move(Converter));
-#endif
+#else
   return std::make_error_code(std::errc::invalid_argument);
+#endif
 }

llvm/unittests/Support/ConvertEBCDICTest.cpp

@@ -41,7 +41,7 @@ static const char AccentE[] = "\xaa\x4a\xb1\xc1\x63\x67\x9e\xc5\x74\x71\x72"
 // String with Cyrillic character ya.
 static const char CyrillicUTF[] = "\xd0\xaf";
 
-TEST(CharSet, FromUTF8) {
+TEST(ConverterEBCDIC, convertToEBCDIC) {
   // Hello string.
   StringRef Src(HelloA);
   SmallString<64> Dst;
@@ -72,7 +72,7 @@ TEST(CharSet, FromUTF8) {
   Dst.clear();
 }
 
-TEST(CharSet, ToUTF8) {
+TEST(ConverterEBCDIC, convertFromEBCDIC) {
   // Hello string.
   StringRef Src(HelloE);
   SmallString<64> Dst;