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

[sorenj]

Summary

Many C++ programmers are unaware that an expression of unsigned - signed will
promote the signed argument to unsigned, and the resulting underflow produces
a large unsigned result, rather than signed negative result.
Hence the frequent errors related to the test x.size() - 1 <= 0
when the container x is empty.

This clang tidy detects signed values being subtracted from unsigned values
and warns the user about the potential error. It is not perfect as it is
not always possible at compile time to reason about code when this comparison
is made.

The warning also suggests a fixit change that will append a "u" to numerical
constants - this makes the implicit cast explicit and signals that the
developer knew what they were doing in a subtraction. In other cases it
suggests the rather abhorrent static_cast<>().

The easiest fix is to not do subtraction at all, just move the operation
to the other side of the comparison where it becomes an addition - which
has none of these surprising properties.

[Eugene.Zelenko]

Please add documentation and mention new check in Release Notes.

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp

11

Unnecessary empty line.

138

Please separate with empty line.

[sorenj]

Address requested whitespace changes.

[lebedev.ri]

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp

22

The check's name is more generic than what it does - it only looks at mixed subtractions in comparisons.
The name implies it looks at all mixed subtractions.

[sorenj]

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp

22

But it does look at all mixed subtractions, line 28 of the unit test shows one such example.

[lebedev.ri]

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp

22

Oh, i didn't spot that one, sorry for the false alarm.

[JonasToth]

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. unsigned short + unsigned short + unsigned short, because of integer promotion).
Just to inform you as it might help with testing or anything like that.

[sorenj]

Friendly ping - anything further I need to do here?

[Eugene.Zelenko]

Documentation and Release Notes entry are still missing..

[sorenj]

• Merge branch 'master' of https://github.com/llvm/llvm-project
• Add documentation and release notes, fix spelling error.

[sorenj]

First time so trying to follow similar recent submits. PTAL.

[Eugene.Zelenko]

clang-tools-extra/docs/ReleaseNotes.rst

106

Please synchronize with first statement in documentation.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst

6–7

This check should be removed.

16

fix-it. Check, not warning.

18

code was intended?

[sorenj]

• Address documentation comments.
• Address documentation comments.

[Eugene.Zelenko]

clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst

18

Please fix double space.

[sorenj]

• Remove double space.

[sorenj]

Anything further needed?

[aaron.ballman]

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 approach for diagnosing the issue? When you ignore the false positives from the test, the only cases not pointed out by -Wsign-compare are the macro cases (which is reasonable behavior to not diagnose on -- the fix for one macro may make other macros invalid).

clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

36–37

I consider this to be a false positive diagnostic -- the 2 is converted from a signed value into an unsigned value and there cannot possibly be a conversion error on that implicit cast.

100

Similarly, I consider this to be a false positive.

[sorenj]

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 2u. This check is targeting a very specific but frequent case of functions that do not guard against containers that might be empty.

Regarding the false positives - I think you are focusing on the semantics of the fix-it which is literally a no-op. The code before and after may be just as wrong, but the salience of the implied conversion is a bit higher. Maybe that's not a good idea for a change that may be applied automatically, even if safe.

In short I'm not sure if you are objecting the principle here, or the implementation. Is this something that can be refined further? I understand the objection in the first case, but what about the second case makes you think this is not a good diagnostic and a false positive? It's the specific case I was targeting: the unprotected subtraction of a non-zero value from a potentially smaller value.

This check was used across our very large code base and was evaluated by examining the code by looking at the context where these warning fired. The vast majority were judged to be true positives. Unfortunately it's not possible to share those results. I will try to run this check on some fraction of the llvm code base and follow up.

[sorenj]

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<uint16_t>(
         0xD800
       | ((((wc & 0x1F0000) >> 16) - 1) << 6)
       |   ((wc & 0x00FC00) >> 10));

the fact that a signed value is being right shifted here surprises me, but looking earlier there's a branch for wc < 0x010000 so we are safe here against wc being 0. So this is a false diagnostic. Still, wc is a uint32_t, it's the 0x1f0000 that converts it to signed. Probably should be 0x1f0000u? Will still false-alarm on this code though unless you use - 1u to make the whole thing unsigned end to end.

the third is in memory.cc

void*
align(size_t alignment, size_t size, void*& ptr, size_t& space)
{
    void* r = nullptr;
    if (size <= space)
    {
        char* p1 = static_cast<char*>(ptr);
        char* p2 = reinterpret_cast<char*>(reinterpret_cast<size_t>(p1 + (alignment - 1)) & -alignment);

here it doesn't make sense for alignment to be zero, and the & -alignment will be zero anyway, so false alarm although some check for alignment > 0 might be an improvement

The last two are in valarray.cpp lines 35 and 43, I've copied a large excerpt here

void
gslice::__init(size_t __start)
{
    valarray<size_t> __indices(__size_.size());
    size_t __k = __size_.size() != 0;
    for (size_t __i = 0; __i < __size_.size(); ++__i)
        __k *= __size_[__i];
    __1d_.resize(__k);
    if (__1d_.size())
    {
        __k = 0;
        __1d_[__k] = __start;
        while (true)
        {
            size_t __i = __indices.size() - 1;
            while (true)
            {
                if (++__indices[__i] < __size_[__i])
                {
                    ++__k;
                    __1d_[__k] = __1d_[__k-1] + __stride_[__i];
                    for (size_t __j = __i + 1; __j != __indices.size(); ++__j)
                        __1d_[__k] -= __stride_[__j] * (__size_[__j] - 1);
                    break;
                }
                else

with some looking I can see that __indices.size() has to be non-zero. It's less clear to me that size_[__j] is strictly positive here and there should probably be some guard against underflow there.

That's a firing rate of about 5/12K lines of code, but I wonder if I were to send patches for these 3 files that silence the warning I wonder how many would be approved.

[aaron.ballman]

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 addressing that issue rather than duplicating the functionality, no?

And, unsigned x = 2; does not trigger a sign conversion warning despite there being a conversion form 2 to 2u.

That should *not* trigger a sign conversion warning because there is no sign conversion. We know the exact value of the RHS and can see that it does not change signs.

This check is targeting a very specific but frequent case of functions that do not guard against containers that might be empty.

We should consider a better name for the check then and limit its utility to those cases. Truthfully, I think that check would be quite useful, but it would almost certainly be a clang static analyzer check to handle control and data flow. e.g., such a check should be able to handle these situations:

size_t count1 = some_container.size() - 1; // Bad if empty
size_t num_elements = some_container.size();
size_t count2 = num_elements - 1; // Bad if empty
if (num_element)
  size_t count3 = num_elements - 1; // Just fine
size_t count4 = std::size(some_container) - 1; // Bad if empty
size_t count5 = std::distance(some_container.begin(), some_container.end()) - 1; // Bad if empty? (Note, there's no type-based sign conversion here)

num_elements + 1;
size_t count6 = num_elements - 1; // Just fine

Regarding the false positives - I think you are focusing on the semantics of the fix-it which is literally a no-op. The code before and after may be just as wrong, but the salience of the implied conversion is a bit higher. Maybe that's not a good idea for a change that may be applied automatically, even if safe.

In short I'm not sure if you are objecting the principle here, or the implementation.

A bit of both. I don't think clang-tidy should get a -Wsign-conversion check -- the frontend handles that, and if there are deficiencies with that handling, we should address those. However, a check that's focused solely on container and container-like situations where an empty container would cause problems seems like a very interesting check that has value. I'm not certain that implementing it in clang-tidy would catch the cases with a reasonable false-positive rate, but it seems like it wouldn't be bad as a static analyzer check instead.

[sorenj]

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 really expects to subtract a signed value from an unsigned value.

[MaskRay]

+@NoQ on comments whether clang static analyzer can catch these cases.

clang++ --analyze a.cc does not warn on a.size()-2 AFAICT.

[NoQ]

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 help of path-sensitive analysis would ultimately allow you to potentially eliminate (in a very precise and fairly principled/non-hacky way) false positives such as your example with .empty() or even this one:

void foo(vector &v) {
  v.push_back(a);
  v.push_back(b);

  // Size is known to be at least 2, therefore overflow never occurs.
  for (size_t i = 0; i < v.size() - 2; ++i) {
    // ...
  }
}

That won't happen immediately though; it'll require some routine work that'll consist in teaching the analyzer facts such as "only empty containers have size 0" or "vectors grow when pushed into". The analyzer would automagically refute such false positives (in all of its checkers!) once it acquires such knowledge. That said, it's still a fairly large amount of routine work, so i'd rather not have you blocked on this and recommend committing into clang-tidy. We can always move or duplicate the check later if you decide to proceed with this approach.

[NoQ]

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 probably become much more quiet and much less useful. So i think this check will work best as an opt-in style/lint guideline. It might still make a bit more sense with path-sensitive analysis but we'll still have to accept the inevitable obvious false positives so it's probably not worth it.

[aminya]

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.

clang-tools-extra/clang-tidy/bugprone/BugproneTidyModule.cpp

 #include "SuspiciousStringCompareCheck.h"
 #include "SwappedArgumentsCheck.h"
 #include "TerminatingContinueCheck.h"
 #include "ThrowKeywordMissingCheck.h"
 #include "TooSmallLoopVariableCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 #include "UndelegatedConstructorCheck.h"
 #include "UnhandledSelfAssignmentCheck.h"
+#include "UnsignedSubtractionCheck.h"
 #include "UnusedRaiiCheck.h"
 #include "UnusedReturnValueCheck.h"
 #include "UseAfterMoveCheck.h"
 #include "VirtualNearMissCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<TooSmallLoopVariableCheck>(
         "bugprone-too-small-loop-variable");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
     CheckFactories.registerCheck<UndelegatedConstructorCheck>(
         "bugprone-undelegated-constructor");
     CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
         "bugprone-unhandled-self-assignment");
+    CheckFactories.registerCheck<UnhandledSelfAssignmentCheck>(
+        "bugprone-unsigned-subtraction");
     CheckFactories.registerCheck<UnusedRaiiCheck>(
         "bugprone-unused-raii");
     CheckFactories.registerCheck<UnusedReturnValueCheck>(
         "bugprone-unused-return-value");
     CheckFactories.registerCheck<UseAfterMoveCheck>(
         "bugprone-use-after-move");
     CheckFactories.registerCheck<VirtualNearMissCheck>(
         "bugprone-virtual-near-miss");

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.h

+//===--- UnsignedSubtractionCheck.h - clang-tidy ----------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGNED_SUBTRACTION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGNED_SUBTRACTION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds cases where a signed value is subtracted from an unsigned value,
+/// a likely cause of unexpected underflow.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-unsigned-subtraction.html
+class UnsignedSubtractionCheck : public ClangTidyCheck {
+ public:
+  UnsignedSubtractionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+ private:
+  llvm::StringSet<> Visited;
+};
+
+}  // namespace bugprone
+}  // namespace tidy
+}  // namespace clang
+
+#endif  // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_UNSIGEND_SUBTRACTION_H

clang-tools-extra/clang-tidy/bugprone/UnsignedSubtractionCheck.cpp

+//===--- UnsignedSubtractionCheck.cpp - clang-tidy --------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "UnsignedSubtractionCheck.h"
+#include "../utils/Matchers.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Tooling/FixIt.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void UnsignedSubtractionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto UnsignedIntType = hasType(isUnsignedInteger());
+  const auto SignedIntType = hasType(isSignedInteger());
+  const auto SizeOf = anyOf(sizeOfExpr(has(type())), sizeOfExpr(has(expr())));
+  const auto UnsignedSubtraction = binaryOperator(
+      hasOperatorName("-"),
+      hasLHS(expr(allOf(UnsignedIntType, unless(SizeOf))).bind("lhs")),
+      hasRHS(implicitCastExpr(
+          hasSourceExpression(anyOf(integerLiteral().bind("literal"),
+                                    expr(SignedIntType).bind("rhs"))))));
+  Finder->addMatcher(UnsignedSubtraction, this);
+
+  // Track comparisons involving unsigned ints, to exclude cases where
+  // the code is (potentially) protected against underflows.
+  Finder->addMatcher(binaryOperator(matchers::isComparisonOperator(),
+                                    hasEitherOperand(UnsignedIntType))
+                         .bind("comparison"),
+                     this);
+
+  // Invocations of container.empty() will exempt checks for container.size().
+  Finder->addMatcher(
+      callExpr(callee(functionDecl(hasName("empty"))), argumentCountIs(0))
+          .bind("call-empty"),
+      this);
+}
+
+void UnsignedSubtractionCheck::check(const MatchFinder::MatchResult &Result) {
+  const char *message =
+      "signed value subtracted from unsigned value is converted to unsigned; "
+      "with possible integer underflow. "
+      "When used in comparisons, consider moving term to other side as a sum "
+      "instead of a subtraction.\n";
+  const SourceManager &SM = *Result.SourceManager;
+  const ASTContext &Ctx = *Result.Context;
+
+  const auto *CallEmpty = Result.Nodes.getNodeAs<CallExpr>("call-empty");
+  if (CallEmpty) {
+    CharSourceRange SourceRange =
+        Lexer::makeFileCharRange(CharSourceRange::getTokenRange(
+                                     CallEmpty->getCallee()->getSourceRange()),
+                                 SM, Ctx.getLangOpts());
+    if (SourceRange.isValid()) {
+      // Calls for foo.empty(), strip off the empty() and replace with size()
+      // to exempt calls to size() from being checked.
+      StringRef CalleeText = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(CallEmpty->getSourceRange()), SM,
+          getLangOpts());
+      Visited.insert(
+          CalleeText.str().substr(0, CalleeText.size() - 7 /* empty() */) +
+          "size()");
+    }
+    return;
+  }
+
+  const auto *Comparison = Result.Nodes.getNodeAs<BinaryOperator>("comparison");
+  if (Comparison) {
+    for (auto side : {Comparison->getLHS(), Comparison->getLHS()}) {
+      if (side->getType()->isUnsignedIntegerType()) {
+        std::string Text = tooling::fixit::getText(*side, Ctx).str();
+        Visited.insert(Text);
+      }
+    }
+    return;
+  }
+
+  const auto *lhs = Result.Nodes.getNodeAs<Expr>("lhs");
+  std::string Text = tooling::fixit::getText(*lhs, Ctx).str();
+  if (Visited.count(Text)) {
+    // This expression has been seen in the context of a comparison, so
+    // presuming it is safe.
+    return;
+  }
+
+  const auto *literal = Result.Nodes.getNodeAs<IntegerLiteral>("literal");
+  const auto *rhs = literal ? literal : Result.Nodes.getNodeAs<Expr>("rhs");
+
+  llvm::APSInt leftConstant;
+  llvm::APSInt rightConstant;
+  if (lhs->isIntegerConstantExpr(leftConstant, Ctx) &&
+      rhs->isIntegerConstantExpr(rightConstant, Ctx)) {
+    // If both values are compile time constants, do not warn.
+    if (llvm::APSInt::compareValues(leftConstant, rightConstant) > 0) return;
+  }
+
+  if (literal) {
+    CharSourceRange SourceRange = Lexer::makeFileCharRange(
+        CharSourceRange::getTokenRange(rhs->getSourceRange()), SM,
+        Ctx.getLangOpts());
+    if (SourceRange.isValid()) {
+      StringRef NumberText = Lexer::getSourceText(
+          CharSourceRange::getTokenRange(rhs->getSourceRange()), SM,
+          getLangOpts());
+      if (NumberText.find_first_not_of("0123456789") == StringRef::npos) {
+        diag(rhs->getBeginLoc(), message)
+            << FixItHint::CreateInsertion(SourceRange.getEnd(), "u");
+        return;
+      }
+    }
+  }
+  // Also handles fall through cases where the literal couldn't be fixed with
+  // a suffix.
+  CharSourceRange SourceRange = Lexer::makeFileCharRange(
+      CharSourceRange::getTokenRange(rhs->getSourceRange()), SM,
+      Ctx.getLangOpts());
+  DiagnosticBuilder Diag = diag(rhs->getBeginLoc(), message);
+  if (SourceRange.isInvalid()) {
+    // An invalid source range likely means we are inside a macro body. A manual
+    // fix is likely needed so we do not create a fix-it hint.
+    return;
+  }
+  Diag << FixItHint::CreateInsertion(
+              SourceRange.getBegin(),
+              "static_cast<unsigned " + rhs->getType().getAsString() + ">(")
+       << FixItHint::CreateInsertion(SourceRange.getEnd(), ")");
+}
+
+}  // namespace bugprone
+}  // namespace tidy
+}  // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

- New :doc:`bugprone-not-null-terminated-result
   result.
 
 - New :doc:`bugprone-signed-char-misuse
   <clang-tidy/checks/bugprone-signed-char-misuse>` check.
 
   Finds ``signed char`` to integer conversions which might indicate a
   programming error.
 
+- New :doc:`bugprone-unsigned-subtraction
+  <clang-tidy/checks/bugprone-unsiged-subtraction>` check.
+
+  Finds expresssions where a signed value is subtracted from an
+  unsigned value, a likely cause of unexpected underflow.
+
 - New :doc:`cert-mem57-cpp
   <clang-tidy/checks/cert-mem57-cpp>` check.
 
   Checks if an object of type with extended alignment is allocated by using
   the default ``operator new``.
 
 - New :doc:`cert-oop58-cpp
   <clang-tidy/checks/cert-oop58-cpp>` check.

clang-tools-extra/docs/clang-tidy/checks/bugprone-unsigned-subtraction.rst

+.. title:: clang-tidy - bugprone-unsigned-subtraction
+
+bugprone-sizeof-expression
+==========================
+
+Finds expressions where a signed value is subtracted from an
+unsigned value, a likely cause of unexpected underflow.
+
+Many programmers are unaware that an expression of unsigned - signed
+will promote the signed argument to unsigned, and if an underflow
+occurs it results in a large positive value. Hence the frequent
+errors related to to tests of ``container.size() - 1 <= 0`` when a
+container is empty.
+
+This check suggest a fix-it change that will append a ``u`` to
+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, a ``static_cast`` is recommended. Heuristics are
+employed to reduce warnings in contexts where the subtraction
+may be safe.
+
+In many cases, the subtraction can be avoided entirely, consider:
+
+.. code-block:: c++
+
+  if (x <= container.size() - 1) { func(container[x]); }
+
+moving the subtraction to the other side avoids the potential
+underflow
+
+.. code-block:: c++
+
+  if (x + 1 <= container.size()) { func(container[x]); }
+

clang-tools-extra/test/clang-tidy/checkers/bugprone-unsigned-subtraction.cpp

+// RUN: %check_clang_tidy %s bugprone-unsigned-subtraction %t
+
+template <class T>
+class vector {
+ public:
+  unsigned size();
+  bool empty();
+};
+
+#define MACRO_MINUS(x) x - 5
+#define MACRO_INT 20
+
+void signedSubtracted() {
+  unsigned x = 5;
+  int yyyy = 2;
+  if (x - yyyy == 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+    // CHECK-FIXES: if (x - static_cast<unsigned int>(yyyy) == 0) {
+    return;
+  }
+  if (0 >= x - yyyy) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+    // CHECK-FIXES: if (0 >= x - static_cast<unsigned int>(yyyy)) {
+    return;
+  }
+  unsigned z = MACRO_MINUS(x);
+  // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: signed value subtracted from
+  z = x - MACRO_INT;
+  // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+  // CHECK-FIXES: z = x - static_cast<unsigned int>(MACRO_INT);
+}
+
+void signedConstantSubtracted() {
+  unsigned x = 5;
+  if (x - 2 > 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: signed value subtracted from
+    // CHECK-FIXES: if (x - 2u > 0) {
+    return;
+  }
+}
+
+void casesThatShouldBeIgnored() {
+  unsigned x = 5;
+  // If the constant used in subtraction is already explicitly unsigned, do not
+  // warn. This is not safe, but the user presumably knows what they are doing.
+  if (x - 2u > 0u) {
+    return;
+  }
+  if (x - 2u > 0) {
+    return;
+  }
+  // sizeof operators are strictly positive for all types, and a constexpr, so
+  // any underflow would happen at compile time, so do not warn.
+  x = sizeof(long double) - 1;
+  // If both sides of the subtraction are compile time constants, don't warn.
+  if (5u - 2 > 0) {
+    return;
+  }
+  constexpr long y = 4;  // NOLINT(runtime/int)
+  if (y - 4 > 0) {
+    return;
+  }
+}
+
+// If the first argument of the subtraction is an expression that was previously
+// used in a comparison, the user is presumed to have done something smart.
+// This isn't perfect, but it greatly reduces false alarms.
+void contextMatters() {
+  unsigned x = 5;
+  if (x < 5) return;
+  if (x - 2 > 0u) {
+    return;
+  }
+}
+
+// For loops with a compartor meet the previously described case, and therefore
+// do not warn if the variable is used in a subtraction. Again not perfect, but
+// this code is complex to reason about and it's best not to generate false
+// alarms.
+unsigned forLoops() {
+  unsigned x;
+  for (unsigned i = 1; i < 5; ++i) {
+    x += i - 1;
+  }
+  return x;
+}
+
+// Testing for an empty container before subtracting from size() is considered
+// to be the same as comparing against the size - it's still possible to fail
+// but reduces the number of false positives.
+void containersEmpty() {
+  vector<int> x;
+  if (!x.empty()) {
+    if (x.size() - 1 > 0) {
+      return;
+    }
+  }
+  vector<double> y;
+  if (y.size() - 1 > 0) {
+    // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: signed value subtracted from
+    // CHECK-FIXES: if (y.size() - 1u > 0) {
+    return;
+  }
+}