clayborg added inline comments.
================
Comment at: lldb/bindings/interface/SBPlatform.i:197-198
+ lldb::SBEnvironment
+ GetEnvironment();
+
----------------
What does it mean to get the environment from a platform? Fetching it from the
remote platform as what ever binary was used to provide the connection?
================
Comment at: lldb/include/lldb/API/SBEnvironment.h:26
+
+ int Size();
+
----------------
labath wrote:
> s/int/size_t
return size_t or uint32_t.
Possibly rename to GetNumEntries() to be consistent with all of the other APIs
that have a size and item accessor.
================
Comment at: lldb/include/lldb/API/SBEnvironment.h:28
+
+ const char *GetEntryAtIndex(int idx);
+
----------------
use size_t or uint32_t (which ever one you pick from Size() above
================
Comment at: lldb/include/lldb/API/SBPlatform.h:157
+ SBEnvironment GetEnvironment();
+
----------------
I know there isn't much header documentation here already, but it is a good
time to start with any new functions. We would need to detail what this
environment would be. Something like: "Return the environment variables
contained in the remote platform connection process."
================
Comment at: lldb/source/API/SBEnvironment.cpp:46
+
+int SBEnvironment::Size() {
+ LLDB_RECORD_METHOD_NO_ARGS(int, SBEnvironment, Size);
----------------
```
size_t SBEnvironment::GetNumEnrtries()
```
================
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]);
+}
----------------
labath wrote:
> 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.
Yes, any C strings returned from the SB API should use
ConstString(cstr).AsCString() as the ConstString puts the string into a string
pool and there is no need to clients to destruct the string or take ownership.
No STL in the SB API please, so no std::string. std::string isn't stable on all
platforms and competing C++ runtimes on different platforms can cause problems.
If we need string ownership, we can introduce SBString as a class.
Please do bounds check this like Pavel requested as the index isn't guaranteed
to be valid. If you bounds check first then your code above could be modified
to return a ConstString().AsCString().
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
