leanil marked 3 inline comments as done.
leanil added inline comments.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:31
+ ->getName()
+ .find("enable_if") != StringRef::npos;
+ };
----------------
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.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:125-126
+ }
+ diag(Ctor->getLocation(), "function %0 can hide copy and move
constructors")
+ << Ctor;
+ }
----------------
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?
================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:70
+ CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
+ "misc-forwarding-reference-overload");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
----------------
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).
Repository:
rL LLVM
https://reviews.llvm.org/D30547
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
