[PATCH] D154186: [clang][DeclPrinter] Fix AST print of delegating constructors

[Aaron Ballman via Phabricator via cfe-commits]

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