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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:119
+       DisabledMove = false;
+  for (const auto *OtherCtor : Ctor->getParent()->ctors()) {
+    if (OtherCtor->isCopyConstructor()) {
----------------
leanil wrote:
> This is the most precise way to formulate the warning message I could come up with.
> The condition for excluding either "copy" or "move" from the warning is to find only disabled instances of the constructor type, and there must be at least one, otherwise the compiler generated constructor (which is not present in this enumeration) can be hidden.
How about:
```
if (OtherCtor->isDeleted() || OtherCtor->getAccess() == AS_private)
  (OtherCtor->isCopyConstructor() ? DisabledCopy : DisabledMove) = true;
else
  (OtherCtor->isCopyConstructor() ? EnabledCopy : EnabledMove) = true;
```


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:136
+       NoMove = DisabledMove && !EnabledMove;
+  if (NoCopy && NoMove) {
+    return;
----------------
Can elide the braces.


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:140
+  diag(Ctor->getLocation(), "constructor accepting a forwarding reference can "
+                            "hide the %select{copy|move|copy and the "
+                            "move}0 constructor%s1")
----------------
copy and the move -> copy and move


================
Comment at: clang-tidy/misc/ForwardingReferenceOverloadCheck.cpp:142
+                            "move}0 constructor%s1")
+      << 2 - NoCopy - 2 * NoMove << NoCopy + NoMove;
+  for (const auto *OtherCtor : Ctor->getParent()->ctors()) {
----------------
The math is correct, but really makes me scratch my head to try to puzzle through it. Why not something along the lines of `Copy && Move ? 2 : (Copy ? 0 : 1)`


Repository:
  rL LLVM

https://reviews.llvm.org/D30547