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

[Kai Nacke via Phabricator via llvm-commits]

Kai added inline comments.


================
Comment at: llvm/CMakeLists.txt:386
 
+set(LLVM_ENABLE_ICONV "ON" CACHE STRING "Use iconv for for character set conversion if available. Can be ON, OFF, or FORCE_ON")
+
----------------
hubert.reinterpretcast wrote:
> Minor nit: s/for for/for;
Changed.


================
Comment at: llvm/include/llvm/Support/CharSet.h:76
+  ///
+  /// The following error codes can occuor, among others:
+  ///   - std::errc::invalid_argument: The requested conversion is not
----------------
hubert.reinterpretcast wrote:
> Minor nit: s/occuor/occur/;
Fixed. Thanks for catching!


================
Comment at: llvm/include/llvm/Support/CharSet.h:90
+
+  CharSetConverter &operator=(CharSetConverter &&Other) {
+    this->Convert = Other.Convert;
----------------
hubert.reinterpretcast wrote:
> If the current object had a cleanup action on entry to the move assignment operator, then that cleanup action should be run prior to replacing the value.
Call to cleanup added.


================
Comment at: llvm/include/llvm/Support/CharSet.h:107
+  ///
+  /// The following error codes can occuor, among others:
+  ///   - std::errc::argument_list_too_long: The result requires more than
----------------
hubert.reinterpretcast wrote:
> Here too: s/occuor/occur/;
Fixed, too.


================
Comment at: llvm/include/llvm/Support/CharSet.h:42
+};
+}
+
----------------
hubert.reinterpretcast wrote:
> ZarkoCA wrote:
> > nit: may be more clear to add the ending comment.
> Minor comment still to address here.
Comment added.


================
Comment at: llvm/lib/Support/CharSet.cpp:135
+        Ch = Ch2 | (Ch << 6);
+        Length--;
+      }
----------------
hubert.reinterpretcast wrote:
> 
Changed.


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

https://reviews.llvm.org/D88741