fdeazeve added inline comments.
================
Comment at: lldb/include/lldb/Utility/StructuredData.h:436
auto array_sp = std::make_shared<Array>();
- collection::const_iterator iter;
- for (iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
+ for (auto iter = m_dict.begin(); iter != m_dict.end(); ++iter) {
auto key_object_sp = std::make_shared<String>();
----------------
Since we are touching this line anyway, could you replace this with
```
for (StringRef key : llvm::make_first_range(m_dict))
```
This has a bit less cognitive burden IMO
================
Comment at: lldb/include/lldb/Utility/StructuredData.h:438
auto key_object_sp = std::make_shared<String>();
- key_object_sp->SetValue(iter->first.AsCString());
+ key_object_sp->SetValue(iter->first());
array_sp->Push(key_object_sp);
----------------
Also since you are touching this line, one could argue for it to be deleted and
folded into `std::make_shared<String>(Key);`
I don't feel strongly about it though, it could also be more appropriate for a
separate commit
================
Comment at: lldb/include/lldb/Utility/StructuredData.h:448
- if (!key.empty()) {
- ConstString key_cs(key);
- collection::const_iterator iter = m_dict.find(key_cs);
----------------
This line in particular also shows why I think this patch is appropriate: even
queries were introducing strings into the pool
================
Comment at: lldb/include/lldb/Utility/StructuredData.h:537
void AddItem(llvm::StringRef key, ObjectSP value_sp) {
- ConstString key_cs(key);
- m_dict[key_cs] = std::move(value_sp);
+ m_dict.insert({key, std::move(value_sp)});
}
----------------
This is not equivalent to the old code, right? The previous code would
overwrite the contents if the key already existed.
================
Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:2035
uint32_t reg;
- if (llvm::to_integer(key.AsCString(), reg))
expedited_register_map[reg] =
----------------
This is another win: the old code was calling `StringRef(const char*)`, which
has to iterate over the entire string to find the null terminating character.
================
Comment at: lldb/source/Plugins/SymbolVendor/MacOSX/SymbolVendorMacOSX.cpp:248
if (do_truncate_remapping_names) {
- FileSpec build_path(key.AsCString());
FileSpec source_path(DBGSourcePath.c_str());
----------------
same win here
================
Comment at: lldb/source/Target/Target.cpp:3866
- s.Printf("%s : %s\n", key.GetCString(),
- object->GetStringValue().str().c_str());
return true;
----------------
we talked about it in private, but I feel like stating this publicly: does
anyone know if this was legal? `object->GetStringValue().str()` creates a
temporary std::string. And then we call `c_str()` and pass that to the Printf
function. Does the temporary std::string have its lifetime extended?
================
Comment at: lldb/source/Utility/StructuredData.cpp:173
+
+ llvm::sort(sorted_entries, [&](const Entry &lhs, const Entry &rhs) -> bool {
+ return lhs.first < rhs.first;
----------------
I thought pairs had `operator <` defined already?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D159313/new/
https://reviews.llvm.org/D159313
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
