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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
