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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
