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

Pavel Labath via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Mon Dec 18 12:22:06 PST 2017

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
+  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.


More information about the lldb-commits mailing list