[PATCH] D154186: [clang][DeclPrinter] Fix AST print of delegating constructors
Aaron Ballman via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 11 11:27:11 PDT 2023
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
More information about the cfe-commits
mailing list