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

[Pavel Labath via Phabricator via lldb-commits]

labath added inline comments.


================
Comment at: include/lldb/Interpreter/Property.h:43
+  ConstString GetName() const { return m_name; }
+  ConstString GetDescription() const { return m_description; }
 
----------------
scott.smith wrote:
> clayborg wrote:
> > 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
> ok but note the original code - it's already storing m_name and m_description as ConstString.  All I'm doing is exposing the underlying type.  Would you prefer I change m_name and m_description to be std::string instead?  Otherwise it won't actually save anything.
> 
> Also note that later uses take the names are put into m_name_to_index in the option parser, which is a UniqueCString, which requires ConstString.  I don't know the code well enough to know whether all options or only some options go through this, so I could see it being worth it to only convert to ConstString at that point (for example, see source/Interpreter/OptionValueProperties.cpp in this review).
I'd suggest keeping these as StringRef as well. The option parser does not do anything performance critical (which probably means it shouldn't be using UniqueCStringMap in the first place, but that is a different discussion...).


================
Comment at: include/lldb/Utility/ConstString.h:481
+  char operator[](size_t Index) const {
+    assert(Index < GetLength() && "Invalid index!");
+    return m_string[Index];
----------------
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 (?)


Repository:
  rL LLVM

https://reviews.llvm.org/D32316