teemperor requested changes to this revision.
teemperor added a comment.
This revision now requires changes to proceed.

In retrospective that's a rather obvious fix. Thanks!

This LGTM, but I think this deserves a comment about why we are setting this. 
Usually LLVM avoids the locale-dependent functions, so this code looks as-is a 
bit out of place in LLVM. A comment pointing out that editline calls 
locale-dependent functions seems good enough. Also two small nits as we're 
anyway touching this code.

FWIW, the Python interpreter runs in the same process as LLDB and Python code 
can change the locale of the current process. So in theory a user could have a 
script that does `locale.setlocale(locale.LC_ALL, "C")` and this way break 
editline again. But that seems such an obscure case that I don't think we 
should bother changing/restoring the locale when we enter/leave editline.



================
Comment at: lldb/tools/driver/Driver.cpp:871
 int main(int argc, char const *argv[]) {
+  ::setlocale(LC_ALL, "");
+  ::setlocale(LC_CTYPE, "");
----------------
Can you make this `std::setlocale`?


================
Comment at: lldb/tools/driver/Driver.cpp:872
+  ::setlocale(LC_ALL, "");
+  ::setlocale(LC_CTYPE, "");
+
----------------
I don't think we need this if we set `LC_ALL`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105779

_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

Reply via email to