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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
