[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.


================
Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+    ConstString func_name = sym_ctx.GetFunctionName();
+    if (func_name == ConstString(function_name) ||
+        alternate_function_name.empty() ||
----------------
JDevlieghere wrote:
> teemperor wrote:
> > JDevlieghere wrote:
> > > I believe someone added an overload for comparing ConstStrings with StringRefs. We shouldn't have to pay the price of creating one here just for comparison. Same below.
> > I really don't want a == overload for StringRef in `ConstString`. The *only* reason for using ConstString is using less memory for duplicate strings and being fast to compare against other ConstStrings. So if we add an overload for comparing against StringRef then code like this will look really innocent even though it essentially goes against the idea of ConstString. Either both string lists are using ConstString or neither does (which I would prefer). But having a list of normal strings and comparing it against ConstStrings usually means that the design is kinda flawed. Then we have both normal string comparisons but also the whole overhead of ConstString.
> > 
> > Looking at this patch we already construct at one point a ConstString from the function name at one point, so that might as well be a ConstString in the first place. If we had an implicit comparison than the conversion here would look really innocent and no one would notice.
> Hmm, you're totally right, I must be confusing this with something else then. 
> The *only* reason for using ConstString is using less memory for duplicate strings and being fast to compare against other ConstStrings.

Just for completeness, a huge use-case for ConstString in LLDB are as keys in DenseMaps and sorted vectors. This avoids redundantly string them in multiple StringMaps.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252