Page MenuHomePhabricator

[clang-tidy] Extension of checker misc-misplaced-widening-cast
ClosedPublic

Authored by baloghadamsoftware on Mar 9 2016, 4:44 AM.

Details

Summary

Existing checker misc-misplaced-widening-cast was extended:

  • New use cases: casted expression as lhs or rhs of a logical comparison or function argument
  • New types: beside int, long and long long various char types, short and int128 added
  • New option to check implicit casts: forgetting a cast is at least as common and as dangerous as misplacing it. This option can be disabled.

This patch depends on AST Matcher patches D17986 and D18243 and also contains fix for checker misc-bool-pointer-implicit-conversion needed because of the fix in the AST Matcher patch.

Diff Detail

Event Timeline

baloghadamsoftware retitled this revision from to Extension of checker misc-misplaced-widening-cast.
baloghadamsoftware updated this object.
baloghadamsoftware added a reviewer: alexfh.
baloghadamsoftware added a subscriber: cfe-commits.
baloghadamsoftware retitled this revision from Extension of checker misc-misplaced-widening-cast to [clang-tidy] Extension of checker misc-misplaced-widening-cast.Mar 9 2016, 4:46 AM

Prerequisites (matchers) are accepted now.

alexfh added inline comments.Mar 22 2016, 4:29 AM
clang-tidy/misc/BoolPointerImplicitConversionCheck.cpp
64–65

The line exceeds 80 columns limit. Please clang-format the change.

clang-tidy/misc/MisplacedWideningCastCheck.cpp
161

isa<T> should be used instead of dyn_cast<T>, when you don't need the resulting pointer.

182–183

isSpecificBuiltinType is not the best tool here. Instead, we could get the builtin type kinds and work with them (CalcType->getAs<BuiltinType>()->getKind() and CastType->getAs<BuiltinType>()->getKind() after checking that both are BuiltinTypes).

test/clang-tidy/misc-misplaced-widening-cast.cpp
28

Please truncate "is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]" in all CHECK-MESSAGES lines but the first one.

41

It makes sense to leave the to '<target type' part in all messages.

clang-tidy/misc/MisplacedWideningCastCheck.cpp
182–183

I do not like it either, just reused the original code and copy-pasted it. I also think that getKind() should be used and instead of the long branches we could do the comparison using relative size table(s).

Required fixes done.

alexfh added inline comments.Mar 24 2016, 7:59 AM
clang-tidy/misc/MisplacedWideningCastCheck.cpp
117

This code shouldn't repeat the lookups. You can, for example, use iterators to keep the results of .find().

It also seems more appropriate to use SmallDenseMap.

182–188

Looks much better now, thanks!

183–188

Please use const auto * to make it obvious it's a pointer.

188

No need for the braces around single-line if bodies.

test/clang-tidy/misc-misplaced-widening-cast.cpp
1–3

We should also have a test with this option turned off.

alexfh requested changes to this revision.Mar 24 2016, 8:00 AM
alexfh edited edge metadata.
This revision now requires changes to proceed.Mar 24 2016, 8:00 AM
baloghadamsoftware edited edge metadata.

Requested revision done.

baloghadamsoftware marked 9 inline comments as done.Mar 25 2016, 6:32 AM
baloghadamsoftware added inline comments.
clang-tidy/misc/MisplacedWideningCastCheck.cpp
117

I changed to SmallDenseMap and its lookup() member function is nice. However it unfortunately lacks an initializer_list constructor.

188

Is this an LLVM style rule? I always learned that it is the safest to use braces even for single-line if bodies.

alexfh accepted this revision.Apr 1 2016, 5:54 PM
alexfh edited edge metadata.

Looks good! Thank you for addressing the comments.

clang-tidy/misc/MisplacedWideningCastCheck.cpp
117

It has a constructor from two iterators, but I'm not sure it'll result in a significantly better code.

188

http://llvm.org/docs/CodingStandards.html is surprisingly inconsistent in this regard, but "no braces around single-line if/for/... bodies" is a more common style in LLVM and, in particular, in clang-tidy code. The problems this style might lead to (statements indented as-if they were under if/for/..., but they actually aren't) are mitigated by the wide use of clang-format.

This revision is now accepted and ready to land.Apr 1 2016, 5:54 PM
xazax.hun closed this revision.Apr 6 2016, 5:10 AM