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

[Michael Buch via Phabricator via lldb-commits]

Michael137 added inline comments.


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1533
 
+std::string
+DWARFASTParserClang::GetTemplateParametersString(const DWARFDIE &die) {
----------------
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`?


================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1579-1580
   while (parent_decl_ctx_die) {
     // The name may not contain template parameters due to simplified template
     // names; we must reconstruct the full name from child template parameter
     // dies via GetTemplateParametersString().
----------------
While you're here mind correcting this?


================
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.
   ///
----------------
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`?


================
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"])
----------------
Nit: technically don't need to run the program to test `image lookup`?


================
Comment at: lldb/test/API/lang/cpp/nested-template/main.cpp:10
+  Outer::Inner<int> oi;
+  // Set breakpoint here
+}
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142413