[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

This looks pretty good overall. I have a bunch of comments, but nothing major.

Could you also please rebase the other patch (D74636 <https://reviews.llvm.org/D74636>) on top of this. I think that a very common use case for this will be taking the platform environment, tweaking it, and then using it to launch a process (i.e. what you are about to do), so it would be good to make sure that flow is sufficiently straight-forward.



================
Comment at: lldb/include/lldb/API/SBEnvironment.h:38
+  /// \return
+  ///     The value of the environment variable or null if not present.
+  lldb::SBStringList GetEntries();
----------------
This doesn't sounds right.


================
Comment at: lldb/include/lldb/API/SBEnvironment.h:41-43
+  /// Adds an environment variable to this object. The input must be a string
+  /// with the format
+  ///   VARIABLE=value
----------------
Maybe explicitly mention that this overwrites any previous value with that name?


================
Comment at: lldb/include/lldb/API/SBEnvironment.h:46
+  /// \return
+  void PutEntry(const char *nameAndValue);
+
----------------
we use the `name_and_value` style elsewhere


================
Comment at: lldb/include/lldb/API/SBTarget.h:97
 
+  /// Return the environment variables or the target.
+  ///
----------------
s/or/of


================
Comment at: lldb/include/lldb/API/SBTarget.h:100
+  /// \return
+  ///     An lldb::SBEnvironment object with the taget's environment variables.
+
----------------
s/taget/target/

Also the concept of a "target's environment variables" is somewhat fuzzy. I take it this is the environment that would be used by default to launch a new process (aka the value of target.env-vars setting)?

And maybe also mention that this is a *copy* of that environment (so any changes made to it don't propagate back anywhere)


================
Comment at: lldb/source/API/SBEnvironment.cpp:52
+                     name);
+  return LLDB_RECORD_RESULT(ConstString(m_opaque_up->lookup(name)).AsCString());
+}
----------------
Can you check what will this return for non-existing variables and for variables which are set to an empty string `FOO=`. I have a feeling this will return the same thing, but I would expect to get a nullptr in the first case, and `""` in the second.


================
Comment at: lldb/source/API/SBEnvironment.cpp:57
+  LLDB_RECORD_METHOD_NO_ARGS(SBStringList, SBEnvironment, GetEntries);
+  auto entries = SBStringList();
+  for (const auto &KV : *m_opaque_up) {
----------------
This is using `auto` just to be fancy. That is not consistent with [[ http://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable | llvm style ]].


================
Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:31-32
+
+        self.build()
+        self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
+        env = self.dbg.GetSelectedTarget().GetEnvironment()
----------------
I don't think you actually need to build an inferior for this. `target = dbg.CreateTarget("")` should be sufficient, I believe.


================
Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:45
+        env.PutEntry("FOO=bar")
+        env.PutEntry("BAR=foo")
+
----------------
As per the other comment, please also test a case like `env.PutEntry("BAZ=")`


================
Comment at: lldb/test/API/python_api/sbenvironment/TestSBEnvironment.py:54
+        for entry in env.GetEntries():
+            self.assertTrue(entry in ["FOO=bar", "BAR=foo"])
----------------
self.assertIn(needle, haystack)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111