xazax.hun 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:
> 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.
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
