[Lldb-commits] [PATCH] D134378: [lldb] Support simplified template names

[Arthur Eubanks via Phabricator via lldb-commits]

aeubanks added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1551
+    }
+    if (!all_template_names.empty()) {
+      all_template_names.append(">");
----------------
labath wrote:
> When can this be empty? Should we still include the `<>` in those cases?
it was empty when we're working with a non-templated die, so a lot of cases. I've added an early exit and changed this to an assert


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2500
+    const llvm::StringRef nameRef = name.GetStringRef();
+    auto it = nameRef.find('<');
+    if (it != llvm::StringRef::npos) {
----------------
labath wrote:
> Michael137 wrote:
> > Is there some other way to determine whether the debug-info was generated from `-gsimple-template-names`? Then we wouldn't have to check the existence of a `<` in the name in multiple places and wouldn't have to do this lookup speculatively. With this change, if we fail to find a template type in the index, we would do the lookup all over again, only to not find it again. Could that get expensive? Just trying to figure out if we can avoid doing this `GetTypes` call twice.
> > 
> > There have been [talks](https://github.com/llvm/llvm-project/issues/58362) about doing a base-name search by default followed by fuzzy matching on template parameters (though in the context of function names). The `simple-template-names` came up as a good motivator/facilitator for doing so. But for that idea to work here we'd have to be able to retrieve from the index by basename, so perhaps out of the scope of what we're trying to do here
> > 
> > tagging people involved in that discussion: @dblaikie @aprantl @jingham @labath
> > 
> > Other than this, generally LGTM
> Negative matches should be fast, and since typically only one of the branches will produce any results, I think this should be fine. Filtering matches in the simplified case would be slower, since you'd have build the full name of all potential candidates (and that could be thousands of instantiations), but I don't know how slow. But that's the price we pay for better filtering (which we don't do right now, but we could start doing afterwards).
Done.

I believe the only difference with `-gsimple-template-names` is that the name doesn't contain the template parameters, so I don't think there's any other way of determining if the debug info was generated from `-gsimple-template-names`.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2577
 
   ConstString name = pattern.back().name;
 
----------------
labath wrote:
> Do we need to do something similar in this FindTypes overload as well?
I was wondering when this code path is even taken, turns out it's only called from lldb-test, and only in two tests:
```
  lldb-shell :: SymbolFile/DWARF/x86/compilercontext.ll
  lldb-shell :: SymbolFile/DWARF/x86/module-ownership.mm
```
so probably doesn't need to be updated?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134378