[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Target/Platform.h:636
 
-  virtual size_t GetEnvironment(StringList &environment);
+  virtual Environment GetEnvironment();
 
----------------
clayborg wrote:
> Do we want to return by value? Not a const reference?
This object can come from the Host::GetEnvironment, which constructs it on-demand. Returning a reference would mean we need to cache a value in the host layer, but that's tricky as the environment can change during runtime, and you want to make sure you always return an up-to-date value.

I don't think this is an issue, as all the returns will be moves, so the situation will be the same as before this patch (where we also created a copy of the environment in the by-ref argument). 


================
Comment at: include/lldb/Target/Target.h:118-119
 
-  size_t GetEnvironmentAsArgs(Args &env) const;
-
-  void SetEnvironmentFromArgs(const Args &env);
+  Environment GetEnvironment() const;
+  void SetEnvironment(Environment env);
 
----------------
clayborg wrote:
> set/get using reference instead of by value?
There is no persistent object object to back the reference for the getter (it's constructed from an OptionValueDictionary). Using a value for the setter is actually better, as that allows me to move-assign the environment object in the LaunchInfo. If the user calls this with a moved object then we get no copies.


================
Comment at: source/API/SBLaunchInfo.cpp:34
+private:
+  Environment::Envp m_envp;
+};
----------------
clayborg wrote:
> Seems like a lot of work to get indexed environment variable access. Should we not make the Environment class use a std::vector instead of a map? Not sure if environment variable order is ever important? Is so then the StringMap isn't what we want to use. I know we have it in our public API so we can't change it now and thus why we need this work around. Just thinking out loud
I was thinking about the importance of variable order as well. I can certainly construct an application that will behave differently depending on the order of environment variables it is given. However, something like that seems unlikely to happen for real.

I actually started working on an order-preserving implementation, but then dropped it when I saw that we are already shoving the variables through a std::map in the OptionValueDictionary in target.env-vars setting. If we manage to drop that (OptionValueEnvironment?), then I think it would make sense to swap out this implementation for an order-preserving one. I would still keep the present map-like interface, as we do key-based lookup in quite a lot of places.


https://reviews.llvm.org/D41359