[PATCH] D96355: [clang-tidy] Extend 'bugprone-easily-swappable-parameters' with optionally considering differently qualified types mixable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:172
+  /// Add the specified qualifiers to the common type in the Mix.
+  MixData operator<<(Qualifiers Quals) const {
+    SplitQualType Split = CommonType.split();
----------------
whisperity wrote:
> aaron.ballman wrote:
> > Hmm, use of `<<` is a bit novel but not entirely indefensible. I guess my initial inclination is that you're combing this information into the mix data and so an overload of `|` was what I was sort of expecting to see here (despite it not really being a bitmask operation). These aren't strong opinions, but I'm curious if you have thoughts.
> I looked at all the possible operator tokens and this seemed the most appropriate. It points to the left, where qualifiers are in LLVM's programming style ("west const" and all that). Because sometimes it's variables that get passed to these functions, seeing from the get-go that it's not meddling with the flags but rather with the types involved seemed appropriate to emphasise.
Okay, that seems reasonable enough to me, thanks!


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:327
+
+  if (LType->isPointerType() && RType->isPointerType()) {
+    // If both types are pointers, and pointed to the exact same type,
----------------
whisperity wrote:
> aaron.ballman wrote:
> > `isAnyPointerType()` for ObjC pointers? Should we care about rvalue references here similar to pointers?
> The reasoning to not be special about && goes back to D95736. If you are given any combination of `T`, `T&`, `T&&` and `const T&` parameters and //some// values of (essentially) `T`, it's only `T` and `const T&` that **always** mix for every potential value of `T`.
> 
> `T&&` won't bind variables, and `T&` won't bind temporaries. So their potential to mix is an inherent property of the //call site// where the mix might happen. If one of them is `T` and the other is `T&` and you happen to pass one variable and one literal, and you happen to swap these two, you'll get a compile error.
> 
> If both parameters are `T&&`, `LType == RType` for a trivial mix catches it early on.
Ah, thank you for the explanation about rvalue references, that's helpful!

As for ObjC... after thinking about it a bit more, I'm on the fence. On the one hand, ObjC is C + extra features, and you can swap parameters in C. On the other hand, ObjC tends to use message sending an awful lot where names the parameters are used at the call site, so this check is less interesting in an ObjC code base in some ways. I think it might make more sense to ignore ObjC for now and try to tackle that in the future (perhaps at user request).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96355