[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth accepted this revision.
amccarth added a comment.
This revision is now accepted and ready to land.

The code is correct, so I'm approving, but I suggest a couple fixes to the comments before committing..



================
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:27
+  // Environment buffer is a list of null-terminated list of strings. It ends
+  // with a double null.
   for (const auto &KV : env) {
----------------
You have an extra "list of" between null-terminated and "strings".  And I think we should point out that it's UTF-16 encoded, so maybe:

```
// The buffer is a list of null-terminated UTF-16 strings, followed by an
// extra L'\0' (two bytes of 0).  An empty environment must have one
// empty string, followed by an extra L'\0'.
```




================
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:39
   buffer.push_back(0);
+  // If the environment was empty, we must insert an extra byte to ensure a
+  // double null.
----------------
"... an extra **two** bytes ..."


================
Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:41
+  // double null.
+  if (env.empty()) {
+    buffer.push_back(0);
----------------
There would be no harm in always adding the extra terminator.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76835