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

[Greg Clayton via Phabricator via lldb-commits]

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