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

[Hubert Tong via Phabricator via llvm-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: llvm/lib/Support/CharSet.cpp:197-199
+  // Convert the string. After all input characters are consumed, call iconv()
+  // with the input parameter set to nullptr, to flush out any partially
+  // converted input characters.
----------------
I don't think there's any "partially converted input characters" to flush. There's only shift sequences that may be unnecessary or unwanted.


================
Comment at: llvm/lib/Support/CharSet.cpp:203-205
+  while ((Ret = iconv(ConvDesc, nullptr, nullptr, &Output, &OutputLength)))
+    if (auto EC = HandleError(Ret))
+      return EC;
----------------
I think forcing return to the initial shift state at this level is too rigid.

As mentioned in https://reviews.llvm.org/D88741?id=379158#inline-1064326, the recommended behaviour for numeric escapes involves no return to the initial shift state prior to the injection of the values specified.

Whether Clang follows that recommendation or not for one platform or the other is a higher level question that should be had in the context of a client of this facility. The facility itself should leave the possibility of resuming with the prior translation's consequent shift state.



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

https://reviews.llvm.org/D88741