[Lldb-commits] [PATCH] D32316: Change UniqueCStringMap to use ConstString as the key

Greg Clayton via Phabricator via lldb-commits lldb-commits at lists.llvm.org
Thu Apr 20 21:00:19 PDT 2017


clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Very close. A few misuses of ConstString and this will be good to go.



================
Comment at: include/lldb/Interpreter/Property.h:43
+  ConstString GetName() const { return m_name; }
+  ConstString GetDescription() const { return m_description; }
 
----------------
This shouldn't be const-ified, Only names of things like variables, enumerators, typenames, paths, and other strings that are going to be uniqued should go into the ConstStringPool


================
Comment at: include/lldb/Symbol/ObjectFile.h:569
   /// Some object files may have an identifier string embedded in them,
-  /// e.g. in a Mach-O core file using the LC_IDENT load command (which 
+  /// e.g. in a Mach-O core file using the LC_IDENT load command (which
   /// is obsolete, but can still be found in some old files)
----------------
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.


================
Comment at: include/lldb/Symbol/ObjectFile.h:576-577
   //------------------------------------------------------------------
-  virtual std::string GetIdentifierString () { 
-      return std::string(); 
+  virtual std::string GetIdentifierString () {
+      return std::string();
   }
----------------
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.


================
Comment at: include/lldb/Symbol/ObjectFile.h:583
   /// "binary" in the core file.  lldb can iterate over the pages looking
-  /// for a valid binary, but some core files may have metadata 
+  /// for a valid binary, but some core files may have metadata
   /// describing where the main binary is exactly which removes ambiguity
----------------
Remove whitespace changes. Do as separate checkin if desired. This helps keep the noise down in the change in case things need to be cherry picked,.


================
Comment at: include/lldb/Symbol/ObjectFile.h:808-811
+  virtual ConstString
+  StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+    return symbol_name;
   }
----------------
This actually doesn't need to change. Since it is merely stripping off parts of the string, this should really stay as a StringRef and it should return a StringRef. Change to use StringRef for return and for argument, or revert.



================
Comment at: include/lldb/Utility/ConstString.h:481
+  char operator[](size_t Index) const {
+    assert(Index < GetLength() && "Invalid index!");
+    return m_string[Index];
----------------
I really don't like the prospect of crashing the debugger if someone calls this with an invalid index. Many people ship with asserts on. I would either make it safe if it is indeed just for reporting or remove the assert.


================
Comment at: source/Interpreter/Property.cpp:252
     if (dump_desc) {
-      llvm::StringRef desc = GetDescription();
-      if (!desc.empty())
+      ConstString desc = GetDescription();
+      if (desc)
----------------
Revert this. Description shouldn't be a ConstString.


================
Comment at: source/Interpreter/Property.cpp:269
     return;
-  llvm::StringRef desc = GetDescription();
+  llvm::StringRef desc = GetDescription().GetStringRef();
 
----------------
Revert this. Description shouldn't be a ConstString.



================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:324-326
+      ConstString key_cstr = m_impl.GetCStringAtIndex(item);
+      if (strstr(type_name.GetCString(), key_cstr.GetCString())) {
+        count += AppendReplacements(type_name, key_cstr, equivalents);
----------------
StringRef is better to use for modifying simple strings and looking for things. Actually looking at this class it is using ConstString all over the place for putting strings together. For creating strings we should use lldb_private::StreamString. For stripping stuff off and grabbing just part of a string, seeing if a string starts with, ends with, contains, etc, llvm::StringRef. So I would vote to just change it back, or fix the entire class to use lldb_private::StreamString


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:345-348
+  uint32_t AppendReplacements(ConstString original,
+                              ConstString matching_key,
                               std::vector<ConstString> &equivalents) {
+    std::string matching_key_str(matching_key.GetCString());
----------------
Ditto


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:354
          match != nullptr; match = m_impl.FindNextValueForName(match)) {
-      std::string target(original);
+      std::string target(original.GetCString());
       std::string equiv_class(match->value.AsCString());
----------------
Ditto


================
Comment at: source/Plugins/Language/CPlusPlus/CPlusPlusLanguage.cpp:363
 // grow indefinitely
 #ifdef ENABLE_CPP_EQUIVALENTS_MAP_TO_GROW
+      Add(original, target_const);
----------------
Ditto


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1811-1816
+ConstString
+ObjectFileELF::StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+  llvm::StringRef sr = symbol_name.GetStringRef();
+  size_t pos = sr.find('@');
+  return ConstString(sr.substr(0, pos));
 }
----------------
Switch to use StringRef as return and for argument or revert.


================
Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.h:155-157
+  lldb_private::ConstString
+  StripLinkerSymbolAnnotations(
+    lldb_private::ConstString symbol_name) const override;
----------------
Switch to use StringRef as return and for argument or revert.


Repository:
  rL LLVM

https://reviews.llvm.org/D32316





More information about the lldb-commits mailing list