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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D76111#1930783 <https://reviews.llvm.org/D76111#1930783>, @labath wrote:

> The new interface is ok for me, but there are two things I want to mention:
>
> - the iteration via `Get{Name,Value}AtIndex` is going to be slow because the underlying map (like most maps) does not have random access iterators. This is the problem I was trying to solve with the `GetEntries`-like API, but I agree that has its issues too.


I am guessing people won't use these much. I didn't know he underlying class used a map. No great solution here unless we modify the underlying class to have a vector with a map of name to index?

> - I agree that forcing a user to manually construct `name=value` strings if he has then as separate entities is not good, but I also don't think we're doing anyone a favor by not providing an api which would accept such strings. The `name=value` format is very universal, and I think users will often have the value in this format already (for example, D74636 <https://reviews.llvm.org/D74636> does). This means that those users would have to implement `=`-splitting themselves before using this class. This is why the internal Environment class provides both kinds of api, and maybe also the reason why the c library provides both `putenv(3)` and `setenv(3)`.

Fine to add functions for this. The user might grab the environment dump in text and want to set all of those env vars, so having the API is fine with me.



================
Comment at: lldb/source/API/SBEnvironment.cpp:59
+                     index);
+  if (index < 0 || index >= GetNumValues())
+    return LLDB_RECORD_RESULT(nullptr);
----------------
labath wrote:
> `index < 0` is not possible now.
yeah, size_t is unsigned on all platforms.


================
Comment at: lldb/source/API/SBEnvironment.cpp:62-73
+      ConstString(std::next(m_opaque_up->begin(), index)->first())
+          .AsCString(""));
+}
+
+const char *SBEnvironment::GetValueAtIndex(size_t index) {
+  LLDB_RECORD_METHOD(const char *, SBEnvironment, GetValueAtIndex, (size_t),
+                     index);
----------------
labath wrote:
> I don't think these need to const-stringified now, given that they are backed by the underlying map. We already have functions which return pointers to internal objects (e.g. SBStream::GetData).
> 
> @clayborg? 
That's a tough one. I would like to think that any "const char *" that comes back from an API that returns strings could just be pointer compared if needed. So I like the idea that for strings that comes out of the API that they are all const-ed and could easily be compared. I am fine with SBStream::GetData() returning its own string because _it_ owns the data. So I would vote to ConstString() any returned results


================
Comment at: lldb/source/API/SBEnvironment.cpp:81
+  if (overwrite || m_opaque_up->find(name) == m_opaque_up->end()) {
+    m_opaque_up->insert_or_assign(name, std::string(value));
+    return LLDB_RECORD_RESULT(true);
----------------
Or do we change the underlying API in the m_opaque_up class to accept an overwrite arg?


================
Comment at: lldb/source/API/SBPlatform.cpp:658-660
+  if (platform_sp) {
+    return LLDB_RECORD_RESULT(SBEnvironment(platform_sp->GetEnvironment()));
+  }
----------------
remove {} (clang code guidelines for single statement if)


================
Comment at: lldb/source/API/SBTarget.cpp:2396-2398
+  if (target_sp) {
+    return LLDB_RECORD_RESULT(SBEnvironment(target_sp->GetEnvironment()));
+  }
----------------
remove {} (clang code guidelines for single statement if)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111