aeubanks added a comment. In D142413#4076749 <https://reviews.llvm.org/D142413#4076749>, @Michael137 wrote:
> In D142413#4075198 <https://reviews.llvm.org/D142413#4075198>, @aeubanks > wrote: > >> there may be an alternate solution involving making some declarations into >> definitions (at all? earlier?), but I don't have that level of understanding >> of lldb > > One idea that came to mind is that instead of creating a dummy template decl > in `GetDIEClassTemplateParams`, you could perhaps construct a > `clang::TemplateArgumentList` from the `TemplateArgument`s stored in > `TemplateParameterInfos` and use the `clang::printTemplateArgumentList` to > stringify the template arguments, passing it the printing-policy which is > used in `TypeSystemClang` to print type-names in your current implementation. this worked perfectly and simplified a lot of code, thanks! ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1533 +std::string +DWARFASTParserClang::GetTemplateParametersString(const DWARFDIE &die) { ---------------- Michael137 wrote: > Michael137 wrote: > > Ah the function was declared but not defined (presumably a leftover from > > https://reviews.llvm.org/D138834) > > > > Can we elaborate in the function docstring how this differs from > > `GetTemplateParameters`? > Meant to say `GetDIEClassTemplateParams` yeah I forgot to remove the declaration in that patch :| ================ Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.h:101 + /// per DIE, we need to start caching the results to prevent unbounded growth + /// of the created clang AST nodes. /// ---------------- Michael137 wrote: > Btw, what's guaranteeing that we don't call this more than once per DIE? Is > there any way we could add a test that would notify us of that kind of > regression? Or perhaps an assert somewhere in the `DWARFASTParserClang`? now we don't create a clang AST node ================ Comment at: lldb/test/API/lang/cpp/nested-template/TestNestedTemplate.py:13 + self.build(dictionary=debug_flags) + lldbutil.run_to_source_breakpoint(self, "// Set breakpoint here", lldb.SBFileSpec("main.cpp")) + self.expect("image lookup -A -t 'Inner<int>'", DATA_TYPES_DISPLAYED_CORRECTLY, substrs=["1 match found"]) ---------------- Michael137 wrote: > Nit: technically don't need to run the program to test `image lookup`? last time I couldn't find the right thing to call to load the binary but not set a breakpoint, but perhaps this works? ================ Comment at: lldb/test/API/lang/cpp/nested-template/main.cpp:10 + Outer::Inner<int> oi; + // Set breakpoint here +} ---------------- Michael137 wrote: > Nit: we typically use `std::puts("Set breakpoint here")` or `return 0` for > source regex breakpoints. The rationale was that breaking on comments isn't > always reliable/future-proof. obsolete now that we don't set a breakpoint Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D142413/new/ https://reviews.llvm.org/D142413 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
