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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
