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