[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Thanks for updating this patch. This seems ok to me (modulo comments here and the other patch), but I think it would be better to move all of the SB changes into that other patch. (the reason I requested this update was to see whether the other patch has all that's needed to implement the given functionality -- and I suspected there would be some bits missing :))



================
Comment at: lldb/bindings/interface/SBLaunchInfo.i:65
     void
-    SetEnvironmentEntries (const char **envp, bool append);
+    SetEnvironmentEntries (const char * const*envp, bool append);
+
----------------
This is going to change the mangled name of the function. Either add a new overload or `const_cast` at the call site. (But if you implement the other comment, neither of those will be needed).


================
Comment at: lldb/include/lldb/API/SBEnvironment.h:62
 
+  lldb_private::Environment &GetInnerEnvironment() const;
+
----------------
Usually, these functions are called `get()` (when they return a pointer) or `ref()` (for a reference).


================
Comment at: lldb/source/API/SBLaunchInfo.cpp:198
+                     (const lldb::SBEnvironment &, bool), env, append)
+  SetEnvironmentEntries(env.GetInnerEnvironment().getEnvp().get(), append);
+}
----------------
This is a pretty long-winded way of implementing this functionality, as the first thing the other function will do is recreate an Environment object. It would be better to have the other function redirect to this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D74636