aaron.ballman requested changes to this revision.
aaron.ballman added inline comments.
This revision now requires changes to proceed.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:31
+ ->getName()
+ .find("enable_if") != StringRef::npos;
+ };
----------------
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`.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:35
+ // Case: pointer to enable_if.
+ if (BaseType->isPointerType()) {
+ BaseType = BaseType->getPointeeType().getTypePtr();
----------------
Reference types as well? Or is that not sensible?
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:53
+}
+AST_MATCHER_P(TemplateTypeParmDecl, hasDefaultArgument,
+ clang::ast_matchers::internal::Matcher<QualType>, TypeMatcher) {
----------------
This seems generally useful and should probably be in ASTMatchers.h. However,
that can be done in a follow-up patch.
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:118
+ // Every parameter after the first must have a default value.
+ if (const auto *Ctor = Result.Nodes.getNodeAs<FunctionDecl>("ctor")) {
+ for (auto Iter = Ctor->param_begin() + 1; Iter != Ctor->param_end();
----------------
I don't think this if statement is required -- `Ctor` should always be found,
no?
================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:125-126
+ }
+ diag(Ctor->getLocation(), "function %0 can hide copy and move
constructors")
+ << Ctor;
+ }
----------------
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.
================
Comment at: clang-tidy/misc/MiscTidyModule.cpp:70
+ CheckFactories.registerCheck<ForwardingReferenceOverloadCheck>(
+ "misc-forwarding-reference-overload");
CheckFactories.registerCheck<MisplacedConstCheck>("misc-misplaced-const");
----------------
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.
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
