scott.smith added a comment.
At a high level, I think there might be a misunderstanding on what I'm
attempting to do. It isn't to convert things that weren't ConstString into
things that are. It is instead to utilize the fact that we have all these
ConstString in order to improve the performance of various operations by
retaining the fact that they are ConstString. While a conversion from
ConstString to StringRef is relatively cheap (at least after a separate change
I have to remove the hashing of the string and the lock operation), the
conversion back to ConstString is expensive. If we pass around StringRef to
functions that need ConstString, then the performance will remain poor.
================
Comment at: include/lldb/Interpreter/Property.h:43
+ ConstString GetName() const { return m_name; }
+ ConstString GetDescription() const { return m_description; }
----------------
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).
================
Comment at: include/lldb/Symbol/ObjectFile.h:808-811
+ virtual ConstString
+ StripLinkerSymbolAnnotations(ConstString symbol_name) const {
+ return symbol_name;
}
----------------
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.
================
Comment at: include/lldb/Utility/ConstString.h:481
+ char operator[](size_t Index) const {
+ assert(Index < GetLength() && "Invalid index!");
+ return m_string[Index];
----------------
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.
================
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);
----------------
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.
================
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());
----------------
clayborg wrote:
> Ditto
In line 352 below, m_impl is a UniqueCStringMap, so all keys are ConstString.
The only way to look up in a UnqiueCStringMap is using ConstString, so
matching_key should remain a ConstString to prevent an unnecessary lookup.
Everything else is following from that.
Repository:
rL LLVM
https://reviews.llvm.org/D32316
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
