aaron.ballman added inline comments.
================
Comment at: clang/test/AST/ast-print-method-decl.cpp:15
+ // CHECK-NEXT: };
+};
----------------
strimo378 wrote:
> aaron.ballman wrote:
> > I'd also like to see test cases along the lines of:
> > ```
> > struct B {
> > template <typename Ty>
> > B(Ty);
> > B(int X) : B((float)X) {}
> > };
> >
> > struct C {
> > C(auto);
> > C(int) : C("") {}
> > };
> > ```
> I fully agree to you that these test cases should also work but that goes
> beyond the scope of a "simple" delegating ctor output fix.
>
> In the current implementation all implicitly specialized templates are output
> and therefore the generated code is not correct C++ anymore, see
> https://godbolt.org/z/34xaoYW5n . I think it is not a good idea to check for
> incorrect C++ code in a test case...
>
> I am currently focusing to get non-templated C++ code output correctly. For
> methods e.g. there are many problems with the correct position of attributes.
> I will address that in one of my next merge request and I want to put all
> method related tests into ast-print-method-decl.cpp . Therefore, I would like
> to keep the name.
> I fully agree to you that these test cases should also work but that goes
> beyond the scope of a "simple" delegating ctor output fix.
Ah, so this is exposing an already existing issue. In that case, let's add the
tests and leave a FIXME comment to improve the behavior in the future (bonus
points for filing an issue) so that it's clear we've noticed there's a problem
rather than looking like an oversight in test coverage. This also ensures that
those forms of delegation don't cause assertions or crashes despite being wrong.
> I am currently focusing to get non-templated C++ code output correctly. For
> methods e.g. there are many problems with the correct position of attributes.
> I will address that in one of my next merge request and I want to put all
> method related tests into ast-print-method-decl.cpp . Therefore, I would like
> to keep the name.
Ah, if there's more functionality to be added to the test in the near future,
the current name is fine by me.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D154186/new/
https://reviews.llvm.org/D154186
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
