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