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