[Lldb-commits] [lldb] [lldb][[DWARFDeclContext] Add function to extract qualified names as vector (PR #77349)

[Greg Clayton via lldb-commits]

================
@@ -68,6 +68,11 @@ class DWARFDeclContext {
 
   const char *GetQualifiedName() const;
 
+  /// Returns a vector of string, one string per entry in the fully qualified
+  /// name. For example, for the DeclContext `A::B::C`, this methods returns
+  /// `{"C", "B", "A"}`
+  llvm::SmallVector<llvm::StringRef> GetQualifiedNameAsVector() const;
----------------
clayborg wrote:

> Out of curiosity, why is a pair of name + tag called a "Compiler Context"?

I believe this was done a long time ago for the -gmodules support where we have a context to a type and we want to find it in the right module's DWARF. So I didn't name it, I just ended up using it for the type lookups. Fine to rename if we have a better name.

> > We could make a using directive for std::vector so we don't have to mention the std::vector all of the time :
> 
> A DWARFDeclContext has a couple of functions that are used elsewhere, so we can't replace it with a vector 1:1. We'd probably still need an abstraction or a few free functions. Also [personal opinion] it is better to have `std::vector` if the abstraction _is_ just vector.

I agree. Maybe just making this a class so it can have some methods might be nice.

> 
> > comparing many matching entries becomes a pointer compare for the string instead of actual string comparisons.
> 
> This is assuming the thing that is compared against is _also_ a ConstString, which is not necessarily the case. This is probably a much longer discussion, but every time I've seen the string comparison argument in favor of using ConstString, I was never able to prove it has any measurable difference. It's obviously hard to prove a negative, but I have notes on experiments where I tried to show they make any difference inside FileSpecs, and could not prove that to be the case. I even made the StringRef::equals function extra slow (no-opt + no length shorcut) and still could not see any performance impact.

Setting a breakpoint by full path on a large project might show some speed differences, but maybe not if StringRef::operator==() is efficient enough to just look at the lengths first so there may be fewer string compares...

For FileSpec objects, I like not having to worry about the lifetime of the string. Switching FileSpec objects over to use StringRef could cause crashes if the object file that owns the strings for the FileSpec gets unloaded. We would need to switch them to use std::string as there are append path component and other things that modify these paths.

> Getting back to the original discussion: if we feel strongly that adding this method is akin to more duplication, I don't mind closing this patch until we can do something about DWARFDeclContext and possibly remove it. In the meantime, the follow up patches to this can instead operate on `DWARFDeclContext::operator[]` + `Entry.Name` without too much extra burden

It would be nice if we had one way to compare contexts that is similar across all symbol file plug-ins, so for me the `std::vector<CompilerContext>` would be nice to use everywhere. It will stop people in SymbolFile plug-ins from having to make a new similar class say for the PDB plug-in. If everyone can use `std::vector<CompilerContext>`, or if we want to make a class that contains `std::vector<CompilerContext>` and has a small API, then we can control things a bit better. Each symbol file plug-in would need to be able to make a `std::vector<CompilerContext>` from items in the debug info and then use a common API. If we make a class, we can control the comparison in methods where we want an exact match or a partial match.

Let me know if this creates a burden or if you think this is the wrong approach


https://github.com/llvm/llvm-project/pull/77349