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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I'm not sure that this is the right API to represent an environment. The environment is more like a dictionary/map than an array. (The internal Environment object *is* a map, though this does not immediately mean the SB one should be too).

Even for your most immediate use case, you'll want to use the map-like properties of the environment (to "merge" the inherited environment and the user-provided values). With an api like this you'd have to implement all of that by yourself.

So, I am wondering if we shouldn't provide a more map-like interface here too. Rough proposal:

  const char *GetEntry(const char *name); // nullptr if not present
  void PutEntry(const char *string); // modeled on putenv(3)
  void SetEntry(const char *name, const char *value, bool overwrite); //modeled on setenv(3), maybe we can skip it if it's not needed now
  SBStringList GetEntries(); // if someone wants to enumerate all of them, maybe also skippable

If we don't want a map-like interface, then maybe we don't actually need a separate class, and an SBStringList would work just fine?

Maybe you could also refactor the other patch to use this new functionality (whatever it ends up being), so that we can see whether it makes sense at least for that use case..



================
Comment at: lldb/include/lldb/API/SBEnvironment.h:26-28
+  int Size();
+
+  const char *GetEntryAtIndex(int idx);
----------------
s/int/size_t


================
Comment at: lldb/source/API/SBEnvironment.cpp:1-2
+//===-- SBEnvironment.cpp
+//-------------------------------------------------------===//
+//
----------------
fix formatting


================
Comment at: lldb/source/API/SBEnvironment.cpp:30-31
+SBEnvironment::SBEnvironment(const Environment &rhs)
+    : m_opaque_up(new Environment()) {
+  *m_opaque_up = rhs;
+}
----------------
new(Environment(rhs))


================
Comment at: lldb/source/API/SBEnvironment.cpp:53
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetEntryAtIndex, (int), idx);
+  return LLDB_RECORD_RESULT(m_opaque_up->getEnvp().get()[idx]);
+}
----------------
This will return a dangling pointer as Envp is a temporary object. It's intended to be used to pass the environment object to execve-like function. 
The current "state of the art" is to ConstStringify all temporary strings that go out of the SB api. (Though I think we should start using std::string).

Also, constructing the Envp object just to fetch a single string out of it is pretty expensive. You can fetch the desired result from the Environment object directly.

Putting it all together, this kind of an API would be implemented via something like:
```
  if (idx >= m_opaque_up->size())
    return nullptr;
  return ConstString(Environment::compose(*std::next(m_opaque_up.begin(), idx)).AsCString();
```

... but I am not sure this is really the right api. More on that in a separate comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111