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 

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 ;).

cfe-commits mailing list

Reply via email to