[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:1558-1560
+            if (template_param_infos.hasParameterPack()) {
+              args = template_param_infos.packed_args->args;
+            }
----------------
dblaikie wrote:
> I think there's some existing problems with lldb here (& gdb, as it happens) - this doesn't specify where the parameter pack is in the argument list (looks like it assumes it's the only template parameters?) - and doesn't handle the possibility of multiple parameter packs in a single parameter list, which I think is possible in some corner cases.
> 
> I think maybe the existing lldb code can handle some non-pack parameters followed by a single pack, but this code assumes it's either non-pack or a pack, never both - so might be more restrictive than the existing limitations? But maybe not - I might've misread/misremembered the other lldb code.
you're right, fixed to check both, and added test case


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1585
+          DWARFDIE parent_decl_ctx_die = die.GetParentDeclContextDIE();
+          // TODO: change this to get the correct decl context parent....
+          while (parent_decl_ctx_die) {
----------------
Michael137 wrote:
> dblaikie wrote:
> > What's incorrect about it at the moment?
> > 
> > Oh, I see this code has just moved around.
> Why move this into `ParseStructureLikeDIE`? Just to remove the need for a mutable `std::string&` param? I guess access to `die`? Might be easier to review (and maintain) as a helper function still
so that we have access to `ParseTemplateParameterInfos`, which calls `ParseTemplateDIE` which creates clang types and stuffs them into `TemplateParameterInfos`, which we use to create the template parameters string.

moved into helper function


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2498-2501
+        llvm::StringRef qualNameTemplateParams =
+            qualName.slice(it, qualName.size());
+        if (templateParams != qualNameTemplateParams)
+          return true;
----------------
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> {
}"
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D134378