This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Move incorrect-roundings to upstream.
ClosedPublic

Authored by hokein on Feb 1 2016, 1:45 AM.

Diff Detail

Event Timeline

hokein updated this revision to Diff 46514.Feb 1 2016, 1:45 AM
hokein retitled this revision from to Move incorrect-roundings to upstream..
hokein updated this object.
hokein added a reviewer: alexfh.
hokein added subscribers: jpienaar, cfe-commits.
alexfh edited edge metadata.Feb 1 2016, 6:46 AM

Nice! See a few comments inline.

clang-tidy/misc/IncorrectRoundings.cpp
39

We should use a more effective way of checking whether the type is a (long)? double or float. There'a builtinType() matcher that we could extend with a narrowing matcher calling BuiltinType::isFloatingPoint() (that can be added to ASTMatchers.h, I think).

51

Since these two .bind() calls are in alternative branches, they can use the same identifier. This would also simplify the code in the callback.

72

I don't think we should recommend lrint, since its behavior can be changed by fesetround(). lround() and llround() are better alternatives, IMO. We could also suggest an automated fix for this.

hokein updated this revision to Diff 46539.Feb 1 2016, 8:04 AM
hokein edited edge metadata.

Address Alex's comments.

hokein marked 3 inline comments as done.Feb 1 2016, 8:14 AM
hokein added inline comments.
clang-tidy/misc/IncorrectRoundings.cpp
39

Done. The ASTMatchers.h file is not in clang-tidy repository, so I temporarily implement isFloatingPoint narrowing matcher here.

Will create a separated patch to clang repository.

51

Just found out there is no need to bind here since check function doesn't use it. Have removed it.

hokein retitled this revision from Move incorrect-roundings to upstream. to [clang-tidy] Move incorrect-roundings to upstream..Feb 2 2016, 1:33 AM
alexfh accepted this revision.Feb 7 2016, 10:27 PM
alexfh edited edge metadata.

Looks good. Thanks!

This revision is now accepted and ready to land.Feb 7 2016, 10:27 PM
hokein updated this revision to Diff 47167.Feb 8 2016, 2:05 AM
hokein marked 2 inline comments as done.
hokein edited edge metadata.

Update test.

This revision was automatically updated to reflect the committed changes.