[PATCH] D95736: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with `typedef` and `const &` diagnostics

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:135
   void sanitize() {
-    assert(Flags != MIX_Invalid && "sanitize() called on invalid bitvec");
-    // TODO: There will be statements here in further extensions of the check.
+    assert(Flags != MIX_Invalid && "sanitise() called on sentinel bitvec");
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:237
 
-  // TODO: Implement more elaborate logic, such as typedef, implicit
-  // conversions, etc.
+  // Dissolve certain type sugars that do not affect the mixability of one type
+  // with the other, and also do not require any sort of elaboration for the
----------------
"Dissolve certain type sugars" has to be one of my new favorite comments. :-D


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:240
+  // user to understand.
+  if (isa<const ParenType>(LType.getTypePtr())) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. LHS is ParenType.\n");
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:245
+  }
+  if (isa<const ParenType>(RType.getTypePtr())) {
+    LLVM_DEBUG(llvm::dbgs() << "--- calculateMixability. RHS is ParenType.\n");
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:289-290
 
+/// Calculates if the reference binds an expression of the given type. This is
+/// true iff 'LRef' is some 'const T &' type, and the 'Ty' is 'T' or 'const T'.
+static MixData isLRefEquallyBindingToType(const TheCheck &Check,
----------------
I think this is reasonable but it does raise a question about implicit conversions, which I suppose is more of a generic question about the check. Should the check consider these to be easily swappable parameter types?
```
struct S {
  S(int);
  operator int() const;
};

void func(S s, int i);
```
given that you can call `func()` like:
```
S s;
int i;

func(i, s);
func(s, i);
```
(Also, does the answer change if `S` has more types it can convert to/from? Or does the answer change if `S` can only convert from a type (or convert to a type) rather than convert both ways?)


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:306
+
+  QualType UnqualifiedReferred = ReferredType;
+  UnqualifiedReferred.removeLocalConst();
----------------
It's not really unqualified since it could still have `volatile` (etc).


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:467-468
+  bool operator()(E El) {
+    auto Insert = CalledWith.insert(std::move(El));
+    return Insert.second;
+  }
----------------



================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:473-474
+
+private:
+  llvm::SmallSet<E, N> CalledWith;
+};
----------------
Might as well hoist this to be above the `public` access specifier (and can drop the `private` access specifier entirely at that point).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95736/new/

https://reviews.llvm.org/D95736