[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

aminya added a comment. I just hit this bug in my code where the subtraction of two size_t values resulted in a very large value. Fortunately, the address sanitizer immediately alerted me about the issue. It would be great to have such a warning in clang-tidy. Repository: rG LLVM Github

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

NoQ added a comment. Mmm, well, on second thought, no, that probably won't be sufficient. As long as we're willing to warn on containers about which we know //absolutely nothing//, we'll still have some of those false positives. And if we stop warning on such containers, the check will

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

NoQ added a comment. In D71607#2112228 , @MaskRay wrote: > +@NoQ on comments whether clang static analyzer can catch these cases. > > `clang++ --analyze a.cc` does not warn on `a.size()-2` AFAICT. Implementing such check in the static analyzer with the

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

MaskRay added subscribers: NoQ, MaskRay. MaskRay added a comment. +@NoQ on comments whether clang static analyzer can catch these cases. `clang++ --analyze a.cc` does not warn on `a.size()-2` AFAICT. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. My colleague pointed out that -Wsigned-conversion will not detect this very frequent mistake for (size_t i = 0; i < v.size() - 1; ++i) It is my contention, and I think it's pretty well substantiated by reviewing cases where this detector fails that no coder ever

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

aaron.ballman added a comment. In D71607#1828055 , @sorenj wrote: > Okay, but as you can see the majority of my test cases are intentionally > false negatives `- -Wsign-conversion` triggers so often than many people > don't use it. Then we should be

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. So, I ran this check against the cxx directory of llvm, it fired 5 times so let's look at the context and disucss: There are two identical spots in locale.cpp, the first is around line 2717 uint16_t t = static_cast( 0xD800 | wc & 0x1F) >>

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. Okay, but as you can see the majority of my test cases are intentionally false negatives `- -Wsign-conversion` triggers so often than many people don't use it. And, `unsigned x = 2;` does not trigger a sign conversion warning despite there being a conversion form 2 to

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

aaron.ballman requested changes to this revision. aaron.ballman added a comment. This revision now requires changes to proceed. Given that the compiler already has `-Wsign-conversion`, I'm not certain what value is added by this check. Can you explain a bit about why this is the correct

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. Anything further needed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj updated this revision to Diff 237471. sorenj added a comment. - Remove double space. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 Files:

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst:17 +constants, thus making the implict cast explicit and signals that +the the code was intended. In cases where the second argument is +not a constant,

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj updated this revision to Diff 237466. sorenj added a comment. - Address documentation comments. - Address documentation comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 Files:

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. First time so trying to follow similar recent submits. PTAL. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 ___ cfe-commits mailing

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

Eugene.Zelenko added inline comments. Comment at: clang-tools-extra/docs/ReleaseNotes.rst:106 + + Finds cases where a signed value is subtracted from an unsigned value, + a likely cause of unexpected underflow. Please synchronize with first statement in

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj updated this revision to Diff 237440. sorenj added a comment. - Merge branch 'master' of https://github.com/llvm/llvm-project - Add documentation and release notes, fix spelling error. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

Eugene.Zelenko added a comment. Documentation and Release Notes entry are still missing.. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 ___ cfe-commits

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj added a comment. Friendly ping - anything further I need to do here? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 ___ cfe-commits mailing list

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

JonasToth added a comment. Somewhat related: https://reviews.llvm.org/D40854 This is a check that tried to enforce not mixing any signed/unsigned arithmetic. there was no feedback from the cppcoreguideline-ppl on how to proceed with edge cases and occassion where mixing is not avoidable (e.g.

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21 + +void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) { + const auto UnsignedIntType = hasType(isUnsignedInteger()); sorenj

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj marked an inline comment as done. sorenj added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21 + +void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) { + const auto UnsignedIntType =

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

lebedev.ri added inline comments. Comment at: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:21 + +void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) { + const auto UnsignedIntType = hasType(isUnsignedInteger()); The

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

sorenj updated this revision to Diff 234294. sorenj added a comment. Address requested whitespace changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71607/new/ https://reviews.llvm.org/D71607 Files:

[PATCH] D71607: [clang-tidy] Add unsigned subtraction warning, with suggestion to convert to unsigned literals.

Eugene.Zelenko added a comment. Please add documentation and mention new check in Release Notes. Comment at: clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp:10 +#include "UnsignedSubtractionCheck.h" + +#include "../utils/Matchers.h"