[Lldb-commits] [PATCH] D75761: [lldb] Fix to get the AST we generate for function templates to be closer to what clang generates and expects

[Raphael Isemann via Phabricator via lldb-commits]

teemperor accepted this revision.
teemperor added a comment.
This revision is now accepted and ready to land.

I only have some minor comments but otherwise I think this can land. The title could use some updating as "Fix to get the AST we generate for function templates to be closer to what clang generates and expects" seems very abstract. What about "[lldb] Remove template parameters from FunctionTemplateDecl names" or something like that?



================
Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:1182
+
+          name = D.getFunctionBaseName(nullptr, &Size);
+        }
----------------
You can just pass a nullptr instead of `&Size` if you don't use the value.


================
Comment at: lldb/test/API/lang/cpp/template-function/TestTemplateFunctions.py:16
         self.build()
-        (_, _, thread, _) = lldbutil.run_to_name_breakpoint(self, "main")
-        frame = thread.GetSelectedFrame()
-        expr = "foo(42)"
+        (self.target, self.process, _, bkpt) = lldbutil.run_to_source_breakpoint(self, '// break here',
+                lldb.SBFileSpec("main.cpp", False))
----------------
This line can just be `lldbutil.run_to_source_breakpoint(self, '// break here', lldb.SBFileSpec("main.cpp"))`, no need to assign self.anything. The SBFileSpec constructor is curious as it seems the constructor without the bool is deprecated since 2010 (?) but we're using it in many other places (as the bool parameter doesn't appear to be documented.....)


================
Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:1
 template<typename T>
 int foo(T t1) {
----------------
labath wrote:
> shafik wrote:
> > labath wrote:
> > > Do we have a test for the spaceship operator?
> > We currently don't support C++2a but when we do we should add a test.
> How is this lack of support "implemented"? Would it make sense to test that we do something reasonable (e.g. ignore it) if we do happen to run into it?
> 
> Given that the new version of this patch doesn't treat the spaceship operator specially, I don't think it needs to/should be a part of this patch, but it is something to think about...
Well the expression evaluator doesn't activate C++20 so we would just not parse the `<=>` as the space ship operator in an expression (and therefore not call it). We could still get it into our AST in which case we would probably handle it as if C++20 was activated (at least on the Clang side, not sure about the other parts of LLDB).

I don't think this review is blocked by this though. We first need to add proper C++20 support and then we can actually test this. But I'm also not opposed to having a test that tries calling <=> and just makes sure that Clang handles it and then errors out without crashing anything.


================
Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:15
+
+int f(int) { return 1; }
+
----------------
I wish this function had a more descriptive name. Maybe something like `FuncWithOverload` and `g` would be `FuncWithoutOverload`. Or a comment that the `f` here is intended to have the same name as `A::f` or something like that. Same for the other single-letter function names in here (as they all seem to serve a special purpose and are not just generic functions).


================
Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:42
+
+struct C {};
+
----------------
This isn't related to `A::C` from what I understand, so could this have another name than `C`?


================
Comment at: lldb/test/API/lang/cpp/template-function/main.cpp:67
+         h(10) + h(1.) +
+         // variadic function
+         var(1) + var(1, 2); // break here
----------------
Can you move the comments what those expressions are testing to `TestTemplateFunctions.py`? If the expression fails it would be good to have the respective comment next to the expression instead of the copy in the .cpp file.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D75761