dblaikie added a comment.
Looking pretty good to me FWIW
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1561
+std::string
+DWARFASTParserClang::GetCPlusPlusQualifiedName(const DWARFDIE &die) {
+ if (!die.IsValid())
----------------
Sorry, when I gave feedback on some pieces of this that were just moved around
rather than new code written in this review - I don't mind the code moving
around without changes (and optionally before/after the move making
improvements, but not necessary).
If possible, might simplify the code review to move the code first/separately
from this change, if the move isn't too meaningless independent of the rest of
the changes?
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498-2501
+ llvm::StringRef qualNameTemplateParams =
+ qualName.slice(it, qualName.size());
+ if (templateParams != qualNameTemplateParams)
+ return true;
----------------
aeubanks wrote:
> dblaikie wrote:
> > And this checks the stringified template params match exactly - so there's
> > opportunity for improvement in the future to compare with more fuzzy
> > matching (I guess we can't use clang's AST because that'd involve making
> > two instances of the type somehow which doesn't make a lot of sense) so
> > that users don't have to spell the template spec exactly the same way clang
> > does (eg: `x<int, int>` versus `x<int,int>` - or other more complicated
> > situations with function pointers, parentheses, casts, etc.
> lldb already requires exact name matching when looking up classes
>
> e.g.
> ```
> $ cat /tmp/a.cc
> template<class T>struct Foo {};
> int main() {
> Foo<int> a;
> Foo<long> b;
> Foo<float> c;
> }
> template<class T>struct Foo {};
> int main() {
> Foo<int> a;
> Foo<long> b;
> Foo<float> c;
> }
> ~/repos/llvm-project/build/cmake$ ./bin/lldb /tmp/a
> (lldb) target create "/tmp/a"
> Current executable set to '/tmp/a' (x86_64).
> (lldb) im loo -t 'Foo< int>'
> (lldb) im loo -t 'Foo<int>'
> 1 match found in /tmp/a:
> id = {0x00000058}, name = "Foo<int>", byte-size = 1, decl = a.cc:1,
> compiler_type = "template<> struct Foo<int> {
> }"
> ```
Yeah, sorry - I understand it requires exact matching currently (because the
index has the exact string in it) - my comment was forward-looking/idle thought
that now that with simplified template names we can/have to lookup by base
name, we have the option to do fuzzier matching when filtering all the base
name matches - not suggesting that it's a regression that this currently does
exact matching.
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
