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

[Greg Clayton via Phabricator via lldb-commits]

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

ok as long as we don't want to return const reference when returning Environment values in getters and setters.



================
Comment at: include/lldb/Target/Platform.h:636
 
-  virtual size_t GetEnvironment(StringList &environment);
+  virtual Environment GetEnvironment();
 
----------------
Do we want to return by value? Not a const reference?


================
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);
 
----------------
set/get using reference instead of by value?


================
Comment at: source/API/SBLaunchInfo.cpp:34
+private:
+  Environment::Envp m_envp;
+};
----------------
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


https://reviews.llvm.org/D41359