[PATCH] D30547: [clang-tidy] Forwarding reference overload in constructors

[András Leitereg via Phabricator via cfe-commits]

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