Page MenuHomePhabricator

[clang-tidy] Add integer division check

Authored by rnkovacs on Jul 27 2017, 12:06 AM.



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.


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;

Diff Detail


Event Timeline

rnkovacs created this revision.Jul 27 2017, 12:06 AM
alexfh added inline comments.Jul 27 2017, 7:55 AM
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 requested changes to this revision.Jul 27 2017, 7:58 AM

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

This revision now requires changes to proceed.Jul 27 2017, 7:58 AM
rnkovacs updated this revision to Diff 108605.Jul 28 2017, 2:07 AM
rnkovacs edited edge metadata.
rnkovacs edited the summary of this revision. (Show Details)

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.

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 updated this revision to Diff 109497.Aug 3 2017, 2:10 AM
rnkovacs edited the summary of this revision. (Show Details)

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 requested changes to this revision.Aug 9 2017, 10:16 AM

A few more nits.

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"?

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.

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

This revision now requires changes to proceed.Aug 9 2017, 10:16 AM
rnkovacs updated this revision to Diff 110539.Aug 10 2017, 2:32 AM
rnkovacs edited edge metadata.
rnkovacs marked 3 inline comments as done.
rnkovacs edited the summary of this revision. (Show Details)

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.

This revision is now accepted and ready to land.Aug 10 2017, 6:00 AM
This revision was automatically updated to reflect the committed changes.