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

[Whisperity via Phabricator via cfe-commits]

whisperity 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");
+
----------------
aaron.ballman wrote:
> 
How in the... Okay, I think I need to up my rebase game because I know for a fact that I fixed BrE to AmE in the parent patch and that patch was merged into the branch this patch was created from and this patch is created against the parent, obviously.


================
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");
----------------
aaron.ballman wrote:
> 
Is `const` respected properly in this case, will it not try to cast it away? Wow... I did not know this!


================
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,
----------------
aaron.ballman wrote:
> 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?)
Yes, yes, yes and yes! That is the whole idea, that is one of the things in which we are "better" than the existing works... and one of the reasons behind this whole "argument swapping" deal is much worse in C++, than let's say Java. (A lot of existing literature deals with Java, or deals with C++ but only in terms of type equivalence, or not considering types -- only arg and param names -- at all.) That one shall be diagnosed, but the whole implicit conversion "mess" is coming in a later patch!
In fact, the patch for implicit conversions is already up, but in a //will be reworked// state.

Unfortunately, I can't seem to find a way to link a particular line of a particular diff, so let's do it manually. The `...-implicits.cpp` test file in D75041 [[ http://reviews.llvm.org/D75041?id=315384 | diff 315384 ]] starting from line 95 shows this being dealt with.

Originally, implicit conversions were warned about when they were only unidirectional (i.e. `int -> S` possible, `S -> int` not possible, `fn(int{}, S{})` warned). However, this created **a lot of** false positives even in the two or three projects I went over manually, so... for the time being, I reworked the patch (only changing like 2 lines at most!) to only deal with //bidirectional// implicit conversions. Because in case you say `fn(5, 5)` it still accepts, but if you have a //valid `s`//, `fn(s, 5)` is a compile error.
(You can check the [[ http://reviews.llvm.org/D75041?id=259324 | earlier diff ]] at the same location for this case.)

This becomes a deep rabbit hole way too fast, because one layer of the analysis is //"warn about `f(T1, T2)` if `f(T2, T1)` is also possible"//. That's the idea I'm trying to sell right now. Saying //"warn about `f(T1, T2)` if there exists some type `Tx` with which `f(Tx, Tx)` is possible"// is a whooole lotta harder to handle. Maybe not impossible, but definitely harder, and with most likely introducing a plethora of weird false positives.


================
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,
----------------
whisperity wrote:
> aaron.ballman wrote:
> > 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?)
> Yes, yes, yes and yes! That is the whole idea, that is one of the things in which we are "better" than the existing works... and one of the reasons behind this whole "argument swapping" deal is much worse in C++, than let's say Java. (A lot of existing literature deals with Java, or deals with C++ but only in terms of type equivalence, or not considering types -- only arg and param names -- at all.) That one shall be diagnosed, but the whole implicit conversion "mess" is coming in a later patch!
> In fact, the patch for implicit conversions is already up, but in a //will be reworked// state.
> 
> Unfortunately, I can't seem to find a way to link a particular line of a particular diff, so let's do it manually. The `...-implicits.cpp` test file in D75041 [[ http://reviews.llvm.org/D75041?id=315384 | diff 315384 ]] starting from line 95 shows this being dealt with.
> 
> Originally, implicit conversions were warned about when they were only unidirectional (i.e. `int -> S` possible, `S -> int` not possible, `fn(int{}, S{})` warned). However, this created **a lot of** false positives even in the two or three projects I went over manually, so... for the time being, I reworked the patch (only changing like 2 lines at most!) to only deal with //bidirectional// implicit conversions. Because in case you say `fn(5, 5)` it still accepts, but if you have a //valid `s`//, `fn(s, 5)` is a compile error.
> (You can check the [[ http://reviews.llvm.org/D75041?id=259324 | earlier diff ]] at the same location for this case.)
> 
> This becomes a deep rabbit hole way too fast, because one layer of the analysis is //"warn about `f(T1, T2)` if `f(T2, T1)` is also possible"//. That's the idea I'm trying to sell right now. Saying //"warn about `f(T1, T2)` if there exists some type `Tx` with which `f(Tx, Tx)` is possible"// is a whooole lotta harder to handle. Maybe not impossible, but definitely harder, and with most likely introducing a plethora of weird false positives.
And once implicit conversions are in, a function definition like `fn(int, const S&)` will say that it can be mixed up because `S::S(int)` is a thing and then an `S{}` can be passed badly to a `const S&`.

There is also another, currently not yet uploaded part of the patch, which deals with whether `fn(int, const int`) (note no `&`!) should be deemed mixable. That one will most likely be behind a user configurable option... Is `memcpy(void* dest, const void* source)` mixable? Not if you have `const void*` as a local variable... Which, given the fact that you told me in D69560 that I should drop the `const` from local variables... means that I'll make the mistake of going `memcpy(src, dst)` more easily, than if I kept my `const`s on my locals. But this is some sort of a more philosophical question.   [[ http://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Ri-unrelated | C++CG says ]] `const T, T` is //not// to be warned about. I say it's still an issue as every non-`const` implicitly converts to `const`. But that's why it's best to leave it up for the user to decide.


================
Comment at: clang-tools-extra/clang-tidy/bugprone/EasilySwappableParametersCheck.cpp:358
 
-      assert(M.flags() != MIX_Invalid && "All flags decayed!");
+      assert(M.flags() != MIX_Invalid && "All flags decayed to sentinel!");
 
----------------
@aaron.ballman  Should I say "invalid" here, or "sentinel" here? (Cf. the assert in `sanitize()`.) It's more of a sentinel value... which is also invalid when you want to consider it a valid enum constant to be used for the intended purpose. Full `0...0` bit pattern means someone somewhere messed up great deal. `0...01` (little endian) bit pattern means we concluded there isn't a possible mix. Etc. with the other bits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95736