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

[David Blaikie via Phabricator via lldb-commits]

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