[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;
+  }
----------------
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.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:127-128
+      << Ctor;
+  for (auto Iter = Ctor->getParent()->ctor_begin();
+       Iter != Ctor->getParent()->ctor_end(); ++Iter) {
+    if (Iter->isCopyConstructor()) {
----------------
You can use a range-based for loop here instead.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:129-137
+    if (Iter->isCopyConstructor()) {
+      diag(Iter->getLocation(), "copy constructor declared here",
+           DiagnosticIDs::Note)
+          << *Iter;
+    } else if (Iter->isMoveConstructor()) {
+      diag(Iter->getLocation(), "move constructor declared here",
+           DiagnosticIDs::Note)
----------------
This can be simplified by using a %select as well.


================
Comment at: docs/clang-tidy/checks/misc-forwarding-reference-overload.rst:48
+default values of the type parameters and the types of the contructor
+parameters. If any part of these types contains "enable_if" in its name, we
+assume the constructor is guarded.
----------------
This is no longer accurate.


Repository:
  rL LLVM

https://reviews.llvm.org/D30547