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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

We need to determine if the objects we return are copies (from SBPlatform and SBTarget), or if they are objects that will modify the environment for the platform and target respectively. If we are returning copies, we need setters on both the platform and target. If they are references, then they will just update the current environment. I think I would rather have the ability to modify them using the returned object, but I am not set on this if everyone else thinks otherwise.

I added many comments in the SBEnvironment header.



================
Comment at: lldb/include/lldb/API/SBEnvironment.h:26
+
+  size_t GetNumEntries();
+
----------------
would GetNumValues() be better? Also, if we have this, we need some accessors that work by index:

```
const char *GetNameAtIndex(size_t);
const char *GetValueAtIndex(size_t);
```

Again, I would rather people not have to split up the name and value themselves if we can avoid it.


================
Comment at: lldb/include/lldb/API/SBEnvironment.h:32
+  ///     The value of the enviroment variable or null if not present.
+  const char *GetEntry(const char *name);
+
----------------
Would get Get(...) be better? Mirror the getenv() call?


================
Comment at: lldb/include/lldb/API/SBEnvironment.h:39
+  ///     The value of the environment variable or null if not present.
+  lldb::SBStringList GetEntries();
+
----------------
If we can get a list of values and edit the SBStringList, do we want a setter?:

```
void SetEntries(SBStringList);
```


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

```
bool Set(const char *name, const char *value, bool overwrite);
```

The bool indicates if it was set. I would rather people not have to make string values that contain "name=value" just to set an env var. This mirrors the setenv() API in libc





================
Comment at: lldb/include/lldb/API/SBEnvironment.h:47
+  void PutEntry(const char *nameAndValue);
+
+protected:
----------------
We need a Unset accessor:

```
bool Unset(const char *name);
```

The returned bool can indicate if the value was removed.


================
Comment at: lldb/include/lldb/API/SBTarget.h:97
 
+  /// Return the environment variables or the target.
+  ///
----------------
labath wrote:
> s/or/of
Maybe better as:

```
/// Return an object that contains the environment that will be used when 
/// launching processes.
```

Then we need to explain if this is a copy, or a value that can be modified. It is can be modified, we should add

```
/// The SBEnvironment object can be used to add or remove environment 
/// variables prior to launching a process.
```

If it is a copy, mention using the SetEnvironment accessor I mention below



================
Comment at: lldb/include/lldb/API/SBTarget.h:100
+  /// \return
+  ///     An lldb::SBEnvironment object with the taget's environment variables.
+
----------------
labath wrote:
> 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)
If this is a copy of the environment, then there should be a set accessor:

```
void SBTarget::SetEnvironment(SBEnvironment);
```

I would be ok with either way (return a copy and have setting, or return the actual object so it can be manipulated). What is the best approach?


================
Comment at: lldb/source/API/SBEnvironment.cpp:52
+                     name);
+  return LLDB_RECORD_RESULT(ConstString(m_opaque_up->lookup(name)).AsCString());
+}
----------------
labath wrote:
> 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.
Yes, we need a test for env vars with that are set with no value and it should return "" (non NULL).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111