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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
