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