[libcxx-commits] [PATCH] D119441: [libc++] Fix locale name construction

[Hubert Tong via Phabricator via libcxx-commits]

hubert.reinterpretcast added inline comments.


================
Comment at: libcxx/src/locale.cpp:570
+  if (one == "")
+    one = setlocale(LC_ALL, "");
+
----------------
hubert.reinterpretcast wrote:
> This call to `setlocale` is not okay. It modifies the current C locale for the program.
For example, on Linux, the name of the locale can be discovered piece-wise by using `_NL_LOCALE_NAME`:
```
locale_t p = newlocale(LC_ALL, "", nullptr);
fprintf(stderr, "%s\n", nl_langinfo_l(_NL_LOCALE_NAME(LC_NUMERIC), p));
```


================
Comment at: libcxx/src/locale.cpp:584
+#if defined(_AIX) || defined(__MVS__)
+  separator = ',';
+#else
----------------
All signs are that the AIX format is space-separated.


================
Comment at: libcxx/src/locale.cpp:589
+
+  separator_pos = other.find(separator);
+  if (separator_pos != string::npos) {
----------------



================
Comment at: libcxx/src/locale.cpp:594-596
+      size_t start_pos = 0;
+#else
+      size_t start_pos = other.find("=") + 1;
----------------



================
Comment at: libcxx/src/locale.cpp:598
+#endif
+      size_t length = separator_pos - start_pos;
+      string token = other.substr(start_pos, length);
----------------
Safer to check that the length is not negative because of unexpected format.


================
Comment at: libcxx/src/locale.cpp:601-602
+      other_cat.push_back(token);
+      other.erase(0, separator_pos + 1);
+      separator_pos = other.find(separator);
+    } while (separator_pos != string::npos);
----------------
Avoid modifying the string all the time when updating a scan position will do.


================
Comment at: libcxx/src/locale.cpp:608
+#if defined(_AIX) && defined(__MVS__)
+    if (other_cat.size() < 5)
+      return "*";
----------------
Although AIX is happy with having 5 here and getting the 6th added after this check, z/OS seems to require 8 (7 at the point of this check).



================
Comment at: libcxx/src/locale.cpp:610
+      return "*";
+#else
+    if (other_cat.size() < 11)
----------------



================
Comment at: libcxx/src/locale.cpp:613
+      return "*";
+    size_t equal_pos = other.find("=");
+    other.erase(0, equal_pos + 1);
----------------



================
Comment at: libcxx/src/locale.cpp:637
+#endif
+      size_t length = separator_pos - start_pos;
+      string token = one.substr(start_pos, length);
----------------
Same comment as for the previous loop.


================
Comment at: libcxx/src/locale.cpp:640
+      one_cat.push_back(token);
+      one.erase(0, separator_pos + 1);
+      separator_pos = one.find(separator);
----------------
Same comment as for the previous loop.


================
Comment at: libcxx/src/locale.cpp:647
+#endif
+    one_cat.push_back(one);
+  } else {
----------------
Should there not be a check that there are at least enough for the indexing based on the category?


================
Comment at: libcxx/src/locale.cpp:687
+      }
+      result = result.substr(0, result.length() - 1);
+#else
----------------



================
Comment at: libcxx/src/locale.cpp:691-693
+               "LC_MESSAGES=" + other_cat[5] + ";" + "LC_PAPER=" + other_cat[6] + ";" + "LC_NAME=" + other_cat[7] +
+               ";" + "LC_ADDRESS=" + other_cat[8] + ";" + "LC_TELEPHONE=" + other_cat[9] + ";" +
+               "LC_MEASUREMENT=" + other_cat[10] + ";" + "LC_IDENTIFICATION=" + other_cat[11];
----------------
These appear to be documented as GNU extensions. The `#else` block here is unlikely to be appropriate on Darwin, BSD, or Windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119441