labath added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1528
+DWARFASTParserClang::GetTemplateParametersString(const DWARFDIE &die) {
+ if (llvm::StringRef(die.GetName()).contains("<"))
+ return std::string();
----------------
reverse the condition and use early exit?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1541
+ llvm::raw_string_ostream os(template_name);
+ arg.dump(os);
+ if (!template_name.empty()) {
----------------
`dump` is a "debugging aid". I guess you should call `print` and pass the
appropriate `PrintingPolicy`. It might just be the default, though maybe we
could also try to make an effort to match the output in the
non-simplified-names case.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1551
+ }
+ if (!all_template_names.empty()) {
+ all_template_names.append(">");
----------------
When can this be empty? Should we still include the `<>` in those cases?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2497
+ // templates parameters, try again with the template parameters stripped
since
+ // with -gsimple-template-names the DT_name may only contain the base name.
+ if (types.Empty()) {
----------------
AT_name ?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498
+ // with -gsimple-template-names the DT_name may only contain the base name.
+ if (types.Empty()) {
+ const llvm::StringRef nameRef = name.GetStringRef();
----------------
I don't think this can be keyed off of `Empty()` for two reasons:
- I believe the user can pass a non-empty type list into this function,
expecting it to append to it
- just because you found one templated type above, it doesn't mean that there
can't be another non-templated type somewhere (if only a part of the binary was
built with simplified names).
I think we should just unconditionally (possibly guarded by a max_matches
check) try both.
================
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) {
----------------
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).
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2577
ConstString name = pattern.back().name;
----------------
Do we need to do something similar in this FindTypes overload as well?
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
