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
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
