leanil marked 3 inline comments as done.
leanil 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:
> > > 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.
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
