[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

brooksmoses added a comment. I have noticed two things when attempting to release LLVM with this revision internally at Google: 1. It's catching real bugs, all in constructors where someone wrote "member_ = member_" when they meant "member_ = member". 2. It's catching at least as many cases of

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

brooksmoses added a comment. Further note: I'm noticing that nearly all the signal is from -Wself-assign-field and all the noise is from -Wself-assign. So that would be one obvious way to make this higher-signal in what's enabled in -Wall, if that were a desire. Repository: rC Clang https

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

brooksmoses added a comment. Some further statistics, now that I've done a full cleanup on our code: 8 actual bugs newly found by -Wself-assign-field. (Thank you!) 2 actual bugs newly found by -Wself-assign 6 instances of redundant code newly found by -Wself-assign 90 (approximately) instances o

[PATCH] D44883: [Sema] Extend -Wself-assign and -Wself-assign-field to warn on overloaded self-assignment (classes)

brooksmoses added a comment. A further concern about this in the general case from the reviewer of one of my test-cleanup changes: The "var = *&var" idiom is not necessarily equivalent to "var = var" in cases of user-defined types, because operator& may be overloaded. Repository: rC Clang h

[PATCH] D45685: [Sema] Add -wtest global flag that silences -Wself-assign for overloaded operators.

brooksmoses added a comment. Thanks for the summary, John. To confirm, I found two examples of bugs involving local variables, as well as the field-based examples. And, indeed, all of the false positives were in unit tests. Repository: rC Clang https://reviews.llvm.org/D45685 _

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

brooksmoses added a comment. So, I have bad news: This causes OpenJDK to segfault. The relevant code is here: https://github.com/openjdk/jdk/blob/master/src/hotspot/share/memory/arena.cpp#L311 void Arena::destruct_contents() { if (UseMallocOnly && _first != NULL) { char* end = _firs

[PATCH] D17993: [CodeGen] Apply 'nonnull' and 'dereferenceable(N)' to 'this' pointer arguments.

brooksmoses added a comment. Aha, okay. I hadn't realized that this optimization had a `-fno-delete-null-pointer-checks` option to disable it. I agree that since that's available there's no call for a rollback. (I'll also make sure upstream bugs are filed for the cases in OpenJDK and Verilat

[PATCH] D131351: [C] Default implicit function pointer conversions diagnostic to be an error

brooksmoses added a comment. For the record, so far we've seen this showing up in the following: - A case in a notoriously warning-heavy third-party library where we'd backported a file from a newer version and didn't quite fix all the internal API mismatches. - A ten-year-old bug in a local pa

[PATCH] D156821: [CodeGen] [ubsan] Respect integer overflow handling in abs builtin

brooksmoses added a comment. Just as an FYI note, this found a bug in one of our random-number generators that was taking a random 32-bit integer and applying abs() to it. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156821/new/ https

[PATCH] D75844: [clang] Set begin loc on GNU attribute parsed attrs

brooksmoses added a comment. FWIW, this now causes Clang to produce an error on this code, when it didn't before: using namespace::foo __attribute__((deprecated("message"))); I discussed this with Richard Smith, who points out that GCC does not accept this and it's not permitted according to

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

brooksmoses added a comment. As a heads up, I'm seeing segfaults on internal code as a result of this change, as well as errors in Eigen's unalignedassert.cpp test (specifically, this line asserts: https://github.com/madlib/eigen/blob/master/test/unalignedassert.cpp#L151). Repository: rG LL

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

brooksmoses added a comment. In D99790#2677919 , @lebedev.ri wrote: > In D99790#2677917 , @brooksmoses > wrote: > >> As a heads up, I'm seeing segfaults on internal code as a result of this >> change, as well as e

[PATCH] D99790: [CGCall] Annotate `this` argument with alignment

brooksmoses added a comment. In D99790#2678674 , @lebedev.ri wrote: > In D99790#2678384 , @brooksmoses > wrote: > >> In any case, thanks for the quick reply, and I'll figure out a small >> reproducer if we find so