[Lldb-commits] [PATCH] D159313: [lldb][NFCI] Remove use of ConstString in StructuredData

[Felipe de Azevedo Piovezan via Phabricator via lldb-commits]

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