aeubanks added inline comments.
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:765
+ m_ast.CreateClassTemplateSpecializationType(class_specialization_decl);
+ return clang_type.GetTypeName(/*BaseOnly*/ true);
+ }
----------------
Michael137 wrote:
> Michael137 wrote:
> > Ok so what we're doing is:
> > 1. Create a `ClassTemplateSpecializationDecl` with an empty basename
> > 2. Return the type-name and since the basename is empty we end up with just
> > the template arguments
> >
> > Why can't we just call `GetTemplateParametersString(die)` instead of
> > creating this function?
> Can confirm that this works locally. Though required moving that function out
> of the DWARFASTParserClang, which seems OK
`GetTemplateParametersString` is specifically only used for
`GetCPlusPlusQualifiedName`, which is used below
```
// For C++, we rely solely upon the one definition rule that says
// only one thing can exist at a given decl context. We ignore the
// file and line that things are declared on.
std::string qualified_name = GetCPlusPlusQualifiedName(die);
```
so it doesn't have to match clang's printing. but for the simple template name
stuff, we are comparing clang-generated names, so everything needs to go
through clang.
I've replaced `GetTemplateParametersString` with this code that goes through
clang as well
================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2988
+ template_params =
+ dwarf_ast->GetForwardDeclarationDIETemplateParams(die);
+ }
----------------
Michael137 wrote:
> So after this call we'll have an unnamed `ClassTemplateDecl` hanging off the
> AST? Doing this inside `FindDefinitionTypeForDWARFDeclContext` instead of
> `ParseStructureLikeDIE` feels a bit off. But will have to understand this
> change a bit deeper to know whether there is something we can do about that.
>
> As a side-note, we seem to be accumulating a lot of blocks around LLDB that
> look like:
> ```
> // Do X because -gsimple-template-names
> if (name.contains('<')) {
> // Do something
> }
> ```
> Still wonder if there isn't some flag that can be set on, e.g., a CU, that
> would tell us this.
the effect is not really per-CU but rather per-DIE because in some cases (where
DWARF roundtripping is lossy, @dblaikie would know best) we still put template
params in the name. so it needs to be checked per-DIE
I agree that it's weird to create clang AST nodes here and then forget about
them, but clang uses a bump allocator and we can't easily delete it. As long as
we call this at most once or twice per DIE, I don't think it's worth caching
the results, but if we do end up using this more, we do need to cache the
created AST nodes. I've added a comment to the docstring.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D138834/new/
https://reviews.llvm.org/D138834
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
