[Lldb-commits] [PATCH] D142413: [lldb] Don't create Clang AST nodes in GetDIEClassTemplateParams

[Arthur Eubanks via Phabricator via lldb-commits]

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