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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
