[clang-tidy] Add integer division check

[rnkovacs]

Summary

Finds cases where integer division in a floating point context is likely to
cause unintended loss of precision.

No reports are made if divisions are part of the following expressions:

• operands of operators expecting integral or bool types,
• call expressions of integral or bool types, and
• explicit cast expressions to integral or bool types,

as these are interpreted as signs of deliberateness from the programmer.

Examples:

float floatFunc(float);
int intFunc(int);
double d;
int i = 42;

// Warn, floating-point values expected.
d = 32 * 8 / (2 + i);
d = 8 * floatFunc(1 + 7 / 2);
d = i / (1 << 4);

// OK, no integer division.
d = 32 * 8.0 / (2 + i);
d = 8 * floatFunc(1 + 7.0 / 2);
d = (double)i / (1 << 4);

// OK, there are signs of deliberateness.
d = 1 << (i / 2);
d = 9 + intFunc(6 * i / 32);
d = (int)(i / 32) - 8;

[alexfh]

docs/clang-tidy/checks/bugprone-integer-division.rst

21–24 ↗

(On Diff #108424)

I'd argue that the presence of abs here doesn't add any signal, since the function has overloads both for integer and floating point types.

[alexfh]

Could you run the check over LLVM+Clang code and post a summary of results here?

[rnkovacs]

I run the check on LLVM-Clang, and got this one hit:

/home/reka/codechecker_dev_env/llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp:1000:43: warning: integer division; possible precision loss [bugprone-integer-division]
  SDValue TWOHW = DAG.getConstantFP(1 << (BW / 2), DL, Op.getValueType());
                                          ^

It warns because getConstantFP() expects a floating-point value as its first argument. Still seems to be a false positive. I might want to add an exception for bitshift operators.

docs/clang-tidy/checks/bugprone-integer-division.rst

21–24 ↗

(On Diff #108424)

Well, I meant specifically the above declared function, but you're right, the naming was misleading, so I changed that.

To be honest, this check doesn't work for most of the std math functions involving template magic. Custom user-defined functions could be mainly targeted.

[rnkovacs]

Uploaded a more thought-out version of the check with more cases covered and hopefully clearer docs. It produces no hits on LLVM&Clang.

[alexfh]

A few more nits.

clang-tidy/bugprone/IntegerDivisionCheck.cpp

40–44 ↗

(On Diff #109497)

The way hasAncestor is used in this code makes me uneasy. Each nested ancestor traversal can potentially slow down matching by a factor of the depth of the AST tree, which may lead to poor performance on certain types of code (nested descendant traversal is much worse though). Some of the overhead may be compensated by memoization, but it's hard to predict where it will actually work.

It's usually better to avoid nested ancestor traversals, if there are good alternatives. Here I don't see a better possibility with matchers, but it's to go one level lower and use RecursiveASTVisitor directly. Without constructing / discovering a problematic case it's hard to tell whether the performance win of switching to RAV is worth the added complexity, so I'm not suggesting to do that yet. Just mentioning a possible issue to be aware of.

51 ↗

(On Diff #109497)

Maybe expand the message to describe the pattern the check matches more precisely? E.g. "result of an integer division is used in a floating point context; possible loss of precision"?

docs/ReleaseNotes.rst

63–64 ↗

(On Diff #109497)

The description is somewhat vague. How about "Finds cases where integer division in floating point context is likely to cause unintended loss of precision."? Same in the docs below.

test/clang-tidy/bugprone-integer-division.cpp

14 ↗

(On Diff #109497)

It's better to truncate repeated fragments of the messages, especially when they exceed 80 characters (better on a word boundary ;).

[rnkovacs]

Thanks for the comments. I improved the docs and truncated the messages in the test file.

We also had concerns about the nested hasAncestor matchers but thought that it might be worth a try. If this solution proves to cause too much of a performance burden I can rewrite it using RAVs.

[alexfh]

LG

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

 //===--- BugproneTidyModule.cpp - clang-tidy ------------------------------===//
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "IntegerDivisionCheck.h"
 #include "SuspiciousMemsetUsageCheck.h"
 #include "UndefinedMemoryManipulationCheck.h"
 
 namespace clang {
 namespace tidy {
 namespace bugprone {
 
 class BugproneModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<IntegerDivisionCheck>(
+        "bugprone-integer-division");
     CheckFactories.registerCheck<SuspiciousMemsetUsageCheck>(
         "bugprone-suspicious-memset-usage");
     CheckFactories.registerCheck<UndefinedMemoryManipulationCheck>(
         "bugprone-undefined-memory-manipulation");
   }
 };
 
 } // namespace bugprone

clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyBugproneModule
   BugproneTidyModule.cpp
+  IntegerDivisionCheck.cpp
   SuspiciousMemsetUsageCheck.cpp
   UndefinedMemoryManipulationCheck.cpp
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
   clangTidyUtils
   clangTooling
   )

clang-tools-extra/trunk/clang-tidy/bugprone/IntegerDivisionCheck.h

+//===--- IntegerDivisionCheck.h - clang-tidy---------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INTEGER_DIVISION_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INTEGER_DIVISION_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+/// Finds cases where integer division in a floating point context is likely to
+/// cause unintended loss of precision.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/bugprone-integer-division.html
+class IntegerDivisionCheck : public ClangTidyCheck {
+public:
+  IntegerDivisionCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_BUGPRONE_INTEGER_DIVISION_H

clang-tools-extra/trunk/clang-tidy/bugprone/IntegerDivisionCheck.cpp

+//===--- IntegerDivisionCheck.cpp - clang-tidy-----------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "IntegerDivisionCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace bugprone {
+
+void IntegerDivisionCheck::registerMatchers(MatchFinder *Finder) {
+  const auto IntType = hasType(isInteger());
+
+  const auto BinaryOperators = binaryOperator(anyOf(
+      hasOperatorName("%"), hasOperatorName("<<"), hasOperatorName(">>"),
+      hasOperatorName("<<"), hasOperatorName("^"), hasOperatorName("|"),
+      hasOperatorName("&"), hasOperatorName("||"), hasOperatorName("&&"),
+      hasOperatorName("<"), hasOperatorName(">"), hasOperatorName("<="),
+      hasOperatorName(">="), hasOperatorName("=="), hasOperatorName("!=")));
+
+  const auto UnaryOperators =
+      unaryOperator(anyOf(hasOperatorName("~"), hasOperatorName("!")));
+
+  const auto Exceptions =
+      anyOf(BinaryOperators, conditionalOperator(), binaryConditionalOperator(),
+            callExpr(IntType), explicitCastExpr(IntType), UnaryOperators);
+
+  Finder->addMatcher(
+      binaryOperator(
+          hasOperatorName("/"), hasLHS(expr(IntType)), hasRHS(expr(IntType)),
+          hasAncestor(
+              castExpr(hasCastKind(CK_IntegralToFloating)).bind("FloatCast")),
+          unless(hasAncestor(
+              expr(Exceptions,
+                   hasAncestor(castExpr(equalsBoundNode("FloatCast")))))))
+          .bind("IntDiv"),
+      this);
+}
+
+void IntegerDivisionCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *IntDiv = Result.Nodes.getNodeAs<BinaryOperator>("IntDiv");
+  diag(IntDiv->getLocStart(), "result of integer division used in a floating "
+                              "point context; possible loss of precision");
+}
+
+} // namespace bugprone
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

* **performance-implicit-cast-in-loop** was renamed to
       <http://clang.llvm.org/extra/clang-tidy/checks/performance-implicit-conversion-in-loop.html>`_
     * **readability-implicit-bool-cast** was renamed to
       `readability-implicit-bool-conversion
       <http://clang.llvm.org/extra/clang-tidy/checks/readability-implicit-bool-conversion.html>`_;
       the check's options were renamed as follows:
       ``AllowConditionalIntegerCasts`` -> ``AllowIntegerConditions``,
       ``AllowConditionalPointerCasts`` -> ``AllowPointerConditions``.
 
+- New `bugprone-integer-division
+  <http://clang.llvm.org/extra/clang-tidy/checks/bugprone-integer-division.html>`_ check
+
+  Finds cases where integer division in a floating point context is likely to
+  cause unintended loss of precision.
+
 - New `readability-static-accessed-through-instance
   <http://clang.llvm.org/extra/clang-tidy/checks/readability-static-accessed-through-instance.html>`_ check
 
   Finds member expressions that access static members through instances and
   replaces them with uses of the appropriate qualified-id.
 
 - Added `modernize-use-emplace.IgnoreImplicitConstructors
   <http://clang.llvm.org/extra/clang-tidy/checks/modernize-use-emplace.html#cmdoption-arg-IgnoreImplicitConstructors>`_

clang-tools-extra/trunk/docs/clang-tidy/checks/bugprone-integer-division.rst

+.. title:: clang-tidy - bugprone-integer-division
+
+bugprone-integer-division
+=========================
+
+Finds cases where integer division in a floating point context is likely to
+cause unintended loss of precision.
+
+No reports are made if divisions are part of the following expressions:
+
+- operands of operators expecting integral or bool types,
+- call expressions of integral or bool types, and
+- explicit cast expressions to integral or bool types,
+
+as these are interpreted as signs of deliberateness from the programmer.
+
+Examples:
+
+.. code-block:: c++
+
+  float floatFunc(float);
+  int intFunc(int);
+  double d;
+  int i = 42;
+
+  // Warn, floating-point values expected.
+  d = 32 * 8 / (2 + i);
+  d = 8 * floatFunc(1 + 7 / 2);
+  d = i / (1 << 4);
+
+  // OK, no integer division.
+  d = 32 * 8.0 / (2 + i);
+  d = 8 * floatFunc(1 + 7.0 / 2);
+  d = (double)i / (1 << 4);
+
+  // OK, there are signs of deliberateness.
+  d = 1 << (i / 2);
+  d = 9 + intFunc(6 * i / 32);
+  d = (int)(i / 32) - 8;

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

 .. title:: clang-tidy - Clang-Tidy Checks
 
 Clang-Tidy Checks
 =================
 
 .. toctree::
    android-cloexec-creat
    android-cloexec-fopen
    android-cloexec-open
    android-cloexec-socket
    boost-use-to-string
+   bugprone-integer-division
    bugprone-suspicious-memset-usage
    bugprone-undefined-memory-manipulation
    cert-dcl03-c (redirects to misc-static-assert) <cert-dcl03-c>
    cert-dcl21-cpp
    cert-dcl50-cpp
    cert-dcl54-cpp (redirects to misc-new-delete-overloads) <cert-dcl54-cpp>
    cert-dcl58-cpp
    cert-dcl59-cpp (redirects to google-build-namespaces) <cert-dcl59-cpp>

clang-tools-extra/trunk/test/clang-tidy/bugprone-integer-division.cpp

+// RUN: %check_clang_tidy %s bugprone-integer-division %t
+
+// Functions expecting a floating-point parameter.
+void floatArg(float x) {}
+void doubleArg(double x) {}
+void longDoubleArg(long double x) {}
+
+// Functions expected to return a floating-point value.
+float singleDiv() {
+  int x = -5;
+  int y = 2;
+  return x/y;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: result of integer division used in
+}
+
+double wrongOrder(int x, int y) {
+  return x/y/0.1;
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: result of integer division used in
+}
+
+long double rightOrder(int x, int y) {
+  return 0.1/x/y; // OK
+}
+
+// Typical mathematical functions.
+float sin(float);
+double acos(double);
+long double tanh(long double);
+
+namespace std {
+  using ::sin;
+}
+
+template <typename T>
+void intDivSin(T x) {
+  sin(x);
+}
+
+int intFunc(int);
+
+struct X {
+  int n;
+  void m() {
+    sin(n / 3);
+// CHECK-MESSAGES: :[[@LINE-1]]:9: warning: result of integer division used in
+  }
+};
+
+void integerDivision() {
+  char a = 2;
+  short b = -5;
+  int c = 9784;
+  enum third { x, y, z=2 };
+  third d = z;
+  char e[] = {'a', 'b', 'c'};
+  char f = *(e + 1 / a);
+  bool g = 1;
+
+  sin(1 + c / (2 + 2));
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of integer division used in
+  sin(c / (1 + .5));
+  sin((c + .5) / 3);
+
+  sin(intFunc(3) / 5);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of integer division used in
+  acos(2 / intFunc(7));
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of integer division used in
+
+  floatArg(1 + 2 / 3);
+// CHECK-MESSAGES: :[[@LINE-1]]:16: warning: result of integer division used in
+  sin(1 + 2 / 3);
+// CHECK-MESSAGES: :[[@LINE-1]]:11: warning: result of integer division used in
+  intFunc(sin(1 + 2 / 3));
+// CHECK-MESSAGES: :[[@LINE-1]]:19: warning: result of integer division used in
+
+  floatArg(1 + intFunc(1 + 2 / 3));
+  floatArg(1 + 3 * intFunc(a / b));
+
+  1 << (2 / 3);
+  1 << intFunc(2 / 3);
+
+#define M_SIN sin(a / b);
+  M_SIN
+// CHECK-MESSAGES: :[[@LINE-1]]:3: warning: result of integer division used in
+
+  intDivSin<float>(a / b);
+// CHECK-MESSAGES: :[[@LINE-1]]:20: warning: result of integer division used in
+  intDivSin<double>(c / d);
+// CHECK-MESSAGES: :[[@LINE-1]]:21: warning: result of integer division used in
+  intDivSin<long double>(f / g);
+// CHECK-MESSAGES: :[[@LINE-1]]:26: warning: result of integer division used in
+
+  floatArg(1 / 3);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of integer division used in
+  doubleArg(a / b);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of integer division used in
+  longDoubleArg(3 / d);
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: result of integer division used in
+  floatArg(a / b / 0.1);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of integer division used in
+  doubleArg(1 / 3 / 0.1);
+// CHECK-MESSAGES: :[[@LINE-1]]:13: warning: result of integer division used in
+  longDoubleArg(2 / 3 / 5);
+// CHECK-MESSAGES: :[[@LINE-1]]:17: warning: result of integer division used in
+
+  std::sin(2 / 3);
+// CHECK-MESSAGES: :[[@LINE-1]]:12: warning: result of integer division used in
+  ::acos(7 / d);
+// CHECK-MESSAGES: :[[@LINE-1]]:10: warning: result of integer division used in
+  tanh(f / g);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of integer division used in
+
+  floatArg(0.1 / a / b);
+  doubleArg(0.1 / 3 / 1);
+
+  singleDiv();
+  wrongOrder(a,b);
+  rightOrder(a,b);
+
+  sin(a / b);
+// CHECK-MESSAGES: :[[@LINE-1]]:7: warning: result of integer division used in
+  acos(f / d);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of integer division used in
+  tanh(c / g);
+// CHECK-MESSAGES: :[[@LINE-1]]:8: warning: result of integer division used in
+
+  sin(3.0 / a);
+  acos(b / 3.14);
+  tanh(3.14 / f / g);
+}