[PATCH] D56731: Add -Wimplicit-ctad warning to diagnose CTAD on types with no user defined deduction guides.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:2129
+def warn_class_template_argument_deduction_no_user_defined_guides : Warning<
+  "using class template argument deduction for %0 that has no user-defined deduction guides" >,
+  InGroup<ImplicitCTADUsage>, DefaultIgnore;
----------------
EricWF wrote:
> rsmith wrote:
> > gromer wrote:
> > > EricWF wrote:
> > > > rsmith wrote:
> > > > > gromer wrote:
> > > > > > I'd prefer a phrasing more along the lines of "using class template argument deduction for %0 that might not intentionally support it". That gives us more room to do things like add an attribute later if necessary, and it helps the user understand why this warning indicates a potential problem.
> > > > > I like that approach; something like "using class template argument deduction for %0 that might not intend to support it" -- or perhaps more directly "%0 might not intend to support class template argument deduction" -- along with a note describing how to syntactically suppress the warning (w"add a deduction guide to suppress this warning" or "use the [[clang::ctad]] attribute to suppress this warning" or whatever we decide is best).
> > > > This sounds like a reasonable change to me. Done.
> > > > 
> > > > I'm not sure an attribute is needed at this point; AFAIK there is no case where a user defined guide can't be added. Even if it's just a dummy guide to suppress the warning. For example:
> > > > 
> > > > ```
> > > > 
> > > > struct allow_ctad_t {
> > > >   allow_ctad_t() = delete;
> > > > };
> > > > 
> > > > template <class T>
> > > > struct TestSuppression {
> > > >   TestSuppression(int, T) {}
> > > > };
> > > > TestSuppression(allow_ctad_t) -> TestSuppression<void>;
> > > > ```
> > > > 
> > > > Also, before we add an attribute, we want to be sure that it's not harmful to allowing users to suppress the warning without actually addressing the root problem (tha implicit CTAD results are often surprising). I would like to see real world examples of types that don't need user-defined guides to work.
> > > > 
> > > > I'm not sure an attribute is needed at this point
> > > 
> > > I agree; I just want to keep the option open in case we decide we need one later.
> > `TestSuppression(allow_ctad_t) -> TestSuppression<void>;` doesn't always work:
> > 
> > ```
> > struct X { X(); };
> > template<typename T = X> struct TestSuppression {
> >   TestSuppression(T);
> > };
> > TestSuppression(allow_ctad_t) -> TestSuppression<void>;
> > TestSuppression x({}); // ok without deduction guide, ill-formed with
> > ```
> > 
> > I think this should work reliably (note that the incomplete parameter type makes the deduction guide always non-viable):
> > 
> > ```TestSuppression(struct AllowCTAD) -> TestSuppression<void>;```
> > 
> > ... and maybe we should suggest that in our note, perhaps with a fix-it hint pointing immediately after the class template definition?
> I'm a bit skeptical of a diagnostic pointing at white space at the end of classes. If we could produce a actionable fixit, then maybe, but what do we put on the right hand side of `->`?
Yeah, the "what goes on the right-hand side" problem is troublesome. What I'm worried about is that people will read the note and add a deduction guide that breaks their deduction semantics, or get annoyed with us because it's not obvious how to add a deduction guide without breaking their deduction semantics or duplicating deduction rules from the constructors.

OK, let's go with this for now and see how users react.


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

https://reviews.llvm.org/D56731