[PATCH] D88741: [SystemZ/z/OS] Add utility class for char set conversion.
Kai Nacke via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 10 13:44:56 PST 2021
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
More information about the llvm-commits
mailing list