[PATCH] D30547: [clang-tidy] Forwarding reference overload in constructors

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:125-126
+    }
+    diag(Ctor->getLocation(), "function %0 can hide copy and move constructors")
+        << Ctor;
+  }
----------------
leanil wrote:
> aaron.ballman wrote:
> > xazax.hun wrote:
> > > aaron.ballman wrote:
> > > > xazax.hun wrote:
> > > > > aaron.ballman wrote:
> > > > > > aaron.ballman wrote:
> > > > > > > leanil wrote:
> > > > > > > > aaron.ballman wrote:
> > > > > > > > > I think a better diagnostic might be: "constructor accepting a universal reference hides the %select{copy|move|both the copy and move}0 %select{constructor{|s}1"
> > > > > > > > > 
> > > > > > > > > And then provide a note ("%select{copy|move}0 constructor declared here") that points to the offending copy and/or move constructor.
> > > > > > > > Without checking actual constructor calls, I would have to make notes on every (non disabled) copy / move constructor, any time I produce a warning. And as the warning already states where the problem lies, the notes would only help people find the copy and move constructors. Do you think that's necessary? 
> > > > > > > The warning states where the forwarding reference constructor is, but it doesn't state where the conflicting constructors are. When we issue diagnostics like that, we generally use notes so that the user can see all of the locations involved -- the user may want to get rid of the other constructors, or they may want to get rid of the forwarding reference constructor. Also, saying "can hide" implies that it isn't hiding anything at all, just that it's possible to do so. Tightening up the wording and showing all of the locations involved solves both problems.
> > > > > > This isn't quite complete. It's still an ambiguous statement to say "it can hide"; it does hide these constructors, and we even know which ones. Emit the notes before you emit the main diagnostic and you can use the `%select` suggested originally to be specific in the diagnostic.
> > > > > We can not say for sure without looking at a concrete call whether a constructor is "hidden" or not. It is always determined during the overload resolution.
> > > > > 
> > > > > This check does not consider the calls, because that way it would always miss the possible misuses if libraries. 
> > > > I can see the logic in that. I guess I'm thinking of it the same way we use the phrase "hidden" when describing code like:
> > > > ```
> > > > struct Base {
> > > >   virtual void f(int);
> > > > };
> > > > 
> > > > struct Derived : Base {
> > > >   void f(double);
> > > > };
> > > > 
> > > > ```
> > > > We claim Derived::f() hides Base::f() without considering the callers.
> > > I see. In that case maybe we should come up with a less confusing term like hijacking overload? The constructors are still part of the overload set, so no hiding as in the standard's nomenclature happens here, but the overload resolution is not doing what the user would expect in these cases. 
> > I'm also fine going back to being somewhat more wishy-washy in our phrasing (can hide).
> > 
> > What do you think about using the %select to specify what can be hidden, rather than always talking about copy and move constructors (one of which may not even be present in the user's source)?
> I think the (assumed) use of a compiler generated copy or move is similarly problematic as in the case of a user defined one, so I would keep mentioning both.
That would be handled automatically anyway (the select I suggested has an option for both). Also, you should have a test for the case involving the implicitly declared constructors.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:127
+      << Ctor;
+  for (auto OtherCtor : Ctor->getParent()->ctors()) {
+    if (OtherCtor->isCopyConstructor() || OtherCtor->isMoveConstructor()) {
----------------
Should be `const auto *` rather than just `auto`.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:131
+           "%select{copy|move}0 constructor declared here", DiagnosticIDs::Note)
+          << OtherCtor->isMoveConstructor() << OtherCtor;
+    }
----------------
This passes in too many arguments to the diagnostic, I think `OtherCtor` should be removed.


Repository:
  rL LLVM

https://reviews.llvm.org/D30547