aaron.ballman added inline comments.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:31
+ ->getName()
+ .find("enable_if") != StringRef::npos;
+ };
----------------
leanil wrote:
> aaron.ballman wrote:
> > This should be using `equals()` rather than `find()` -- otherwise a name
> > like `enable_if_awesome()` would trigger the match. You should also make
> > sure the name is in the `::std` namespace so that you can't trigger it with
> > `foo::enable_if` or `foo::std::enable_if`.
> My intention with using `find()` was that if a user has an
> `enable_if_awesome()`, then it seems more likely to be similar to the std
> version, than to have completely different purpose.
> And as a bonus `find()` also finds `std::enable_if_t` in one check.
We don't typically use a heuristic approach like that -- instead, we often let
the user specify their own list of names explicitly, as an option.
================
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:
> > 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.
================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:70
+ CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
+ "misc-forwarding-reference-overload");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
----------------
leanil wrote:
> aaron.ballman wrote:
> > I wonder if the name might be slightly misleading -- I've always understood
> > these to be universal references rather than forwarding references. I don't
> > have the Meyers book in front of me -- what nomenclature does he use?
> >
> > Once we pick the name, we should use it consistently in the source code
> > (like the file name for the check and the check name), the documentation,
> > and the release notes.
> Meyers calls them universal references, but it's //forwarding reference// in
> the standard (14.8.2.1).
Hmm, the terms are a bit too new to really get a good idea from google's ngram
viewer, but the search result counts are:
Google:
"universal reference" : 270,000
"forwarding reference" : 9650
Stack Overflow:
universal reference : 3016
forwarding reference: 1654
So I think that these are probably more well-known as universal references,
despite the standard's nomenclature being forwarding reference.
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
