[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class
Pavel Labath via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Thu Mar 19 02:39:34 PDT 2020
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
More information about the lldb-commits
mailing list