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

[Scott Smith via Phabricator via lldb-commits]

scott.smith added inline comments.


================
Comment at: include/lldb/Symbol/ObjectFile.h:808-811
+  virtual ConstString
+  StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+    return symbol_name;
   }
----------------
scott.smith wrote:
> clayborg wrote:
> > 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.
> > 
> The only user of StripLinkerSymbolAnnotations immediately converts it to a ConstString.  Changing this back to using StringRef would mean an extra lookup for architectures that do not override this function.
reverting this has no measurable performance impact on my test, so even though the caller has a ConstString, and will convert the result to a ConstString, I will revert this.


================
Comment at: include/lldb/Utility/ConstString.h:481
+  char operator[](size_t Index) const {
+    assert(Index < GetLength() && "Invalid index!");
+    return m_string[Index];
----------------
labath wrote:
> scott.smith wrote:
> > clayborg wrote:
> > > 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.
> > This code is copied from StringRef, which is what lldb used before this change.  Do you still want me to revert the assert?
> > 
> > Though IMO it should be changed to <=, not <, so that the null terminator can be read safely.
> I think that asserting here is fine... There's no way you can make code that likes to tread off the end of an array safe by changing this function.
> 
> However, I am not really fond of the idea of ConstString taking over StringRef functionality. I think we should stick to requiring stringref conversions when doing string manipulation. Maybe if you reduce the number of ConstString occurences according to other comments, this will not be that necessary anymore (?)
The indexing is used in Symtab::InitNameIndexes, where is dealing with ConstString.  IMO converting to a StringRef there is a bad idea, as it means there will be two variables representing the same thing (one ConstString, one StringRef), which makes it easy to update one and not the other.

But there is no performance difference, at least if review https://reviews.llvm.org/D32306 is committed (which removes hashing and locking when getting the length).  Otherwise it's a >1% penalty to convert to StringRef in InitNameIndexes.



================
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);
----------------
scott.smith wrote:
> clayborg wrote:
> > 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
> I can revert this fragment, but just note this won't affect the number of ConstStrings that are generated.  m_impl has ConstStrings in it, and type_name is a ConstString, so yes they can be converted to StringRef just to call StringRef::contains, or we can leave them as ConstString and utilize strstr.  Or, I can add ConstString::contains so the interface to ConstString and StringRef is more similar.
> 
turns out this code is unused, filing a separate review to remove it.


Repository:
  rL LLVM

https://reviews.llvm.org/D32316