[PATCH] D35932: [clang-tidy] Add integer division check

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

A few more nits.



================
Comment at: clang-tidy/bugprone/IntegerDivisionCheck.cpp:40-44
+          hasAncestor(
+              castExpr(hasCastKind(CK_IntegralToFloating)).bind("FloatCast")),
+          unless(hasAncestor(
+              expr(Exceptions,
+                   hasAncestor(castExpr(equalsBoundNode("FloatCast")))))))
----------------
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.


================
Comment at: clang-tidy/bugprone/IntegerDivisionCheck.cpp:51
+  const auto *IntDiv = Result.Nodes.getNodeAs<BinaryOperator>("IntDiv");
+  diag(IntDiv->getLocStart(), "integer division; possible precision loss");
+}
----------------
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"?


================
Comment at: docs/ReleaseNotes.rst:63-64
+
+  Finds cases of integer divisions that seem to alter the meaning of the
+  surrounding code.
 
----------------
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.


================
Comment at: test/clang-tidy/bugprone-integer-division.cpp:14
+  return x / y - 1;
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: integer division; possible precision loss [bugprone-integer-division]
+}
----------------
It's better to truncate repeated fragments of the messages, especially when they exceed 80 characters (better on a word boundary ;).


https://reviews.llvm.org/D35932