[libcxx-commits] [PATCH] D98207: [SystemZ][z/OS] Missing wchar functions libc++

[Daniel McIntosh via Phabricator via libcxx-commits]

DanielMcIntosh-IBM added a comment.

Immediately after hitting submit I notice a few more things. Added them as comments too



================
Comment at: libcxx/src/support/ibm/mbsnrtowcs.cpp:39-40
+  while (source_remaining) {
+      if (dst && dest_converted >= max_dest_chars)
+          break;
+      // Converts one multi byte character.
----------------
This could also probably be made part of the loop condition. (Don't forget to invert it properly when you do)


================
Comment at: libcxx/src/support/ibm/mbsnrtowcs.cpp:47
+      // Don't do anything to change errno from here on.
+      if (char_size > 0) {
+          source_remaining -= char_size;
----------------
again, char_size is unsigned, so this won't behave as you've described in the error case.


================
Comment at: libcxx/src/support/ibm/mbsnrtowcs.cpp:47
+      // Don't do anything to change errno from here on.
+      if (char_size > 0) {
+          source_remaining -= char_size;
----------------
DanielMcIntosh-IBM wrote:
> again, char_size is unsigned, so this won't behave as you've described in the error case.
As in `wcsnrtombs`, this should also probably be inverted to convert it to an early-exit


================
Comment at: libcxx/src/support/ibm/mbsnrtowcs.cpp:50
+          source_converted += char_size;
+          ++dest_converted;
+          continue;
----------------
Probably easier to increment `dst` directly here. You'll have to do so behind an extra `if (dst)`, but in exchange you can get rid of the ternary operator on line 45, and get rid of a state variable.


================
Comment at: libcxx/src/support/ibm/mbsnrtowcs.cpp:58
+  if (dst) {
+      if (have_result && result == terminated_sequence)
+          *src = NULL;
----------------
I think that we may be able to get rid of `have_result` here too, but I'm not as sure about this one, since `result` is initialized to 0, and `terminated_sequence == static_cast<size_t>(0)`.


================
Comment at: libcxx/src/support/ibm/wcsnrtombs.cpp:44-45
+      // char_size contains the size of the multi-byte-sequence converted.
+      // Otherwise, char_size is -1 and wcrtomb() sets the errno.
+      if (char_size > 0) {
+          if (c == L'\0') {
----------------
char_size is actually not `-1` but rather `(size_t)-1`, and size_t is an unsigned type, so this test won't behave as you've described in the error case.


================
Comment at: libcxx/src/support/ibm/wcsnrtombs.cpp:53
+              dest_remaining -= char_size;
+          dest_converted += char_size;
+          continue;
----------------
Since dst is a pointer and not a pointer-to-a-pointer, wouldn't it be easier to just add `char_size` directly to `dst` and do away with `dest_converted`? You don't even need to add an extra `if (dst)` since you have one on line 51


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

https://reviews.llvm.org/D98207