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

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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 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)`.



================
Comment at: lldb/source/API/SBEnvironment.cpp:53
+  return LLDB_RECORD_RESULT(
+      ConstString(m_opaque_up->lookup(name)).AsCString(""));
+}
----------------
Please reuse the result of `m_opaque_up->find(name)` here to avoid double lookup.


================
Comment at: lldb/source/API/SBEnvironment.cpp:59
+                     index);
+  if (index < 0 || index >= GetNumValues())
+    return LLDB_RECORD_RESULT(nullptr);
----------------
`index < 0` is not possible now.


================
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);
----------------
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? 


================
Comment at: lldb/source/API/SBEnvironment.cpp:80-81
+                     overwrite);
+  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);
----------------
how about `if(overwrite) insert_or_assign(name, value) else try_emplace(name, value)`? (avoiding two map lookups)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76111