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