new clang-tidy checker misc-long-cast

[danielmarjamaki]

Summary

This is a new checker for clang-tidy.

This checker will warn when there is a explicit redundant cast of a calculation
result to a bigger type. If the intention of the cast is to avoid loss of
precision then the cast is misplaced, and there can be loss of precision.
Otherwise the cast is ineffective.

No warning is written when it is seen that the cast is ineffective.

I tested this on debian projects.. and I see quite little noise. in 1212 projects I got 76 warnings. The cast is either ineffective or there is loss of precision in all these cases. I don't see warnings where I think the warning is wrong. But I don't want to warn when it can be determined that the cast is just ineffective, I think that the perfect checker would be better at determining this.

[Eugene.Zelenko]

docs/clang-tidy/checks/misc-long-cast.rst

6 ↗

(On Diff #45235)

Please use check instead of checker for documentation consistency.

[Eugene.Zelenko]

Clang-tidy has 6 cast related checks. May be this is good time to introduce dedicated category for them?

[LegalizeAdulthood]

Why not supply a fixit that removes the cast?

[danielmarjamaki]

In D16310#330367, @LegalizeAdulthood wrote:

Why not supply a fixit that removes the cast?

I am skeptic. There are different valid fixes.

Example code:

l = (long)(a*1000);

Fix1:

l = ((long)a * 1000);

Fix2:

l = (a * (long)1000);

Fix3:

l = (a * 1000L);

[danielmarjamaki]

Fixed review comment; s/checker/check/

[danielmarjamaki]

In D16310#330312, @Eugene.Zelenko wrote:

Clang-tidy has 6 cast related checks. May be this is good time to introduce dedicated category for them?

I am not against this.

I don't know which ones you are thinking about.. but if a check is not in the misc category now then it should probably not be moved.

I can do this.. but I would prefer to do it separately. If there is agreement that this should be done I can create a new revision that move the cast checks. I suggest that is done before this long-cast checker is committed.

[LegalizeAdulthood]

If you state what the check does, then

In D16310#331054, @danielmarjamaki wrote:

In D16310#330367, @LegalizeAdulthood wrote:

Why not supply a fixit that removes the cast?

I am skeptic. There are different valid fixes.

Example code:

l = (long)(a*1000);

Fix1:

l = ((long)a * 1000);

Fix2:

l = (a * (long)1000);

Fix3:

l = (a * 1000L);

The way I see it, the check is complaining about the pointless cast and pointing the finger at the beginning of the cast. To me, my expectation is that the suggested fix is none of the options you gave but instead:

l = (a*1000);

In other words, the cast is superfluous for the code as written. Omitting the cast directly expresses the code as written without unnecessary casting. Because the check can't know your intent, it hints that you may have intended something else, but for what was written, this is the semantics.

If we want to get into detecting numeric overflow, then we're talking some kind of run-time assisted check and it's beyond the scope of clang-tidy.

Basically, I look at this unnecessary cast as clutter. I'm fine with static analysis that warns about intermediate expressions possibly overflowing when assigned to a "larger" type, but to me that sounds like something for the static analyzer and not for tidy.

In other words, I think of clang-tidy as a refactoring tool, not a static analysis tool.

[LegalizeAdulthood]

clang-tidy/misc/LongCastCheck.cpp

43 ↗

(On Diff #45378)

Prefer anonymous namespace over static to scope visibility.

97 ↗

(On Diff #45378)

Why don't you check for casting a float expression to a double or long double?

Isn't this the exact same issue?

If so, add a test case for casting a float expression to double and a test case for casting a double expression to a long double.

clang-tidy/misc/MiscTidyModule.cpp

61 ↗

(On Diff #45378)

The documentation describes this check as one that looks for a cast to a "bigger type", but the name of the check implies that it only works for casts to long.

The name of the check should be made more generic to reflect reality.

Perhaps misc-redundant-cast-to-larger-type or misc-redundant-bigger-type-cast?

docs/clang-tidy/checks/misc-long-cast.rst

11 ↗

(On Diff #45378)

Please add an example for another type other than long, such as casting a float expression to a double.

[danielmarjamaki]

In D16310#331538, @LegalizeAdulthood wrote:

If you state what the check does, then

In D16310#331054, @danielmarjamaki wrote:

In D16310#330367, @LegalizeAdulthood wrote:

Why not supply a fixit that removes the cast?

I am skeptic. There are different valid fixes.

Example code:

l = (long)(a*1000);

Fix1:

l = ((long)a * 1000);

Fix2:

l = (a * (long)1000);

Fix3:

l = (a * 1000L);

The way I see it, the check is complaining about the pointless cast and pointing the finger at the beginning of the cast. To me, my expectation is that the suggested fix is none of the options you gave but instead:

l = (a*1000);

In other words, the cast is superfluous for the code as written. Omitting the cast directly expresses the code as written without unnecessary casting. Because the check can't know your intent, it hints that you may have intended something else, but for what was written, this is the semantics.

If we want to get into detecting numeric overflow, then we're talking some kind of run-time assisted check and it's beyond the scope of clang-tidy.

Basically, I look at this unnecessary cast as clutter. I'm fine with static analysis that warns about intermediate expressions possibly overflowing when assigned to a "larger" type, but to me that sounds like something for the static analyzer and not for tidy.

In other words, I think of clang-tidy as a refactoring tool, not a static analysis tool.

Why is there a cast in the first place? It is unlikely that the programmer added a useless cast for no reason.

I say that it looks like the programmer wanted to avoid loss of precision.

If we remove the useless cast there will still be loss of precision. And the warnings go away so maybe a sloppy programmer thinks that all is good.

I tested this on debian projects.. and I see quite little noise. in 1212 projects I got 76 warnings.

In my opinion the proper fix for these are to cast the expressions properly.

[danielmarjamaki]

clang-tidy/misc/LongCastCheck.cpp

43 ↗

(On Diff #45378)

As I read the LLVM coding standards we prefer static for functions.

http://llvm.org/docs/CodingStandards.html#anonymous-namespaces

97 ↗

(On Diff #45378)

in theory yes.. but somehow that feels strange to me. yes there will possibly be loss of precision in some decimals, that is normal when using floating point numbers. if such loss of precision would be unwanted then float should be avoided to start with.

so I do agree in theory but I don't think I would feel good about adding such warnings.

clang-tidy/misc/MiscTidyModule.cpp

61 ↗

(On Diff #45378)

Yes I agree.. will fix..

I used long because that is the typical case that I envision.

[alexfh]

clang-tidy/misc/LongCastCheck.cpp

21 ↗

(On Diff #45378)

Any reason to limit this to returnStmt, varDecl and assignment? This pattern can appear in any expression and lead to equally incorrect results.

22 ↗

(On Diff #45378)

Why only c-style casts? The problem applies to static_cast as well. Not sure how likely a reinterpret_cast is to appear in this situation, maybe it should be handled as well.

23 ↗

(On Diff #45378)

Why other operators are not considered here? Subtraction, for example, can suffer the same problem.

66 ↗

(On Diff #45378)

No else after return, please.

71 ↗

(On Diff #45378)

Please remove the extra line breaks.

100 ↗

(On Diff #45378)

Ctx or Context, please.

clang-tidy/misc/MiscTidyModule.cpp

61 ↗

(On Diff #45378)

How about misc-misplaced-widening-cast?

docs/clang-tidy/checks/misc-long-cast.rst

11 ↗

(On Diff #45378)

Is the use of two colons intended?

test/clang-tidy/misc-long-cast.cpp

1 ↗

(On Diff #45378)

Please add tests with templates: casting to or from a dependent type shouldn't trigger the warning.

[danielmarjamaki]

clang-tidy/misc/LongCastCheck.cpp

21 ↗

(On Diff #45378)

Yes. There could be some extra pattern we want to look for later. But I don't want to warn in general. Example:

int A,B,C;
long ABC = (long)(A * B) + C;

That code makes perfect sense if the calculation A*B won't overflow and you want that the addition is done with long precision.

22 ↗

(On Diff #45378)

ok, I will handle that too in next patch.

23 ↗

(On Diff #45378)

yes. true. the ~ operator also. I can't say how noisy that would be but I will test it.

clang-tidy/misc/MiscTidyModule.cpp

61 ↗

(On Diff #45378)

I already changed.. but I like misc-misplaced-widening-cast better so I will change again..

[alexfh]

clang-tidy/misc/LongCastCheck.cpp

21 ↗

(On Diff #45378)

Then you don't need to repeat the inner matcher. Assign it to an auto variable and then use it in the other matchers:

auto CastExpr = cStyleCastExpr(...);
Finder->addMatcher(returnStmt(has(CastExpr)));
Finder->addMatcher(varDecl(has(CastExpr)));
...

55 ↗

(On Diff #45378)

nit: Braces are not needed around one-line if bodies.

61 ↗

(On Diff #45378)

Please insert braces around the if body here: it's hard to see its scope otherwise.

[LegalizeAdulthood]

In D16310#332200, @danielmarjamaki wrote:

Why is there a cast in the first place? It is unlikely that the programmer added a useless cast for no reason.

If this has universally been your experience on a code base, then I say you should count your blessings that you have worked only with such good programmers!

Sadly, I have encountered many code bases where such silly things were written. Sometimes people just write the cast because that is the return type of the function.

At any rate, the check simply cannot divine programmer intent from the source code alone, which is why for clang-tidy (not static analyzer) purposes, my expectation is that clang-tidy would say "this cast is redundant, so I suggest you remove it". Hence the fixit removes the cast.

Only a human being can look at the change suggested by clang-tidy and say "hmm.... looks like the correct thing here is that there was some loss of precision in the intermediate result, so I should fix that instead".

If a developer is going to blindly accept the output of clang-tidy, then there is nothing we can do about that. (A case in point: I am currently reviewing thousands of changes proposed by google-readability-braces-around-statements; it has quite a few bugs and can actually generate syntactically invalid code from its suggested fixits!) At a bare minimum, clang-tidy should never offer fixits that change the meaning of the code and my suggested fixit does not do that. The other proposed fixits discussed on this thread do change the meaning of the code by changing the precision of the intermediate calculations and are therefore not refactorings. They are bug fixes. Without dynamic analysis, only a human can decide on their validity by considering the larger context of the code.

Again, my feeling is that clang-tidy should be a refactoring tool when it comes to fixits -- the suggested changes should never change the semantic meaning of the code.

[LegalizeAdulthood]

clang-tidy/misc/LongCastCheck.cpp

97 ↗

(On Diff #45378)

For floating-point quantities, when I think of the term "precision" I am thinking of the number of bits allocated to the mantissa. This may or may not be correct terminology according to floating-point experts, but from what I can tell it seems to agree with how the term is used on wikipedias article on floating-point.

So while digits are technically lost when we add a small floating-point quantity to a large floating-point quantity (the large quantity gobbles up all the bits of the mantissa and the small quantity has its least-significant mantissa bits discarded), the precision of the result isn't changed -- the number of bits in the mantissa is the same for the result as it was for the inputs.

To then take a quantity of N bits of mantissa and cast that to a quantity of M bits of mantissa where M > N seems just as pointless as the approach of casting an int to a long. The extra tokens for casting do absolutely nothing and are redundant as far as the computation as written is concerned.

clang-tidy/misc/MiscTidyModule.cpp

61 ↗

(On Diff #45378)

Yes, Alex's name suggestion is better than mine. Yay code reviews!

[danielmarjamaki]

In D16310#331538, @LegalizeAdulthood wrote:

If you state what the check does, then

In D16310#331054, @danielmarjamaki wrote:

In D16310#330367, @LegalizeAdulthood wrote:

Why not supply a fixit that removes the cast?

I am skeptic. There are different valid fixes.

Example code:

l = (long)(a*1000);

Fix1:

l = ((long)a * 1000);

Fix2:

l = (a * (long)1000);

Fix3:

l = (a * 1000L);

The way I see it, the check is complaining about the pointless cast and pointing the finger at the beginning of the cast. To me, my expectation is that the suggested fix is none of the options you gave but instead:

l = (a*1000);

I expect that this warning will in most cases be fixed by moving the cast.

I believe there is often a bug in such code.

If you want that we hide these bugs by removing the casts I can do it.. but I personally think that is wrong.

And yes clang-tidy does not have static analysis. that would not help much to determine proper fix anyway - if we can see there is overflow then should it be fixed with fix 1,2,3 or is the overflow intentional.

[LegalizeAdulthood]

I find this an interesting discussion. I do not mean to imply that my remarks constitute any sort of demand that this check produce my suggestion for a fixit.

In D16310#333416, @danielmarjamaki wrote:

I expect that this warning will in most cases be fixed by moving the cast.

For a seasoned C++ developer having authored the offending code, I agree. However, I have seen lots of code where things were done that were just unnecessary.

I believe there is often a bug in such code.

I think it depends on who was writing it.

If you want that we hide these bugs by removing the casts I can do it.. but I personally think that is wrong.

I'm simply saying that if it is going to suggest a fixit, then the fixit suggested should be one that doesn't change the meaning of the code as written.

I agree that such a suggested fixit would not be the correct change in all cases, since clang-tidy can't know the original intent of the author.

However, suppose we had code like this:

long f(int a) {
  return a*1000;
}

This code is not going to issue any warnings about potential loss of precision, but by simply adding the cast on the result expression, suddenly I'm getting warnings about possible loss of precision.

In both cases, the precision loss is a potentiality.

The check assumes that placing the cast there is an indicator of a bug.

When I look at such code with a redundant cast, I would infer the opposite: the programmer is adding stuff that isn't necessary.

Maybe our differing attitude on what the cast implies is a reflection of the difference in our experiences in working on different code bases. Am I just so unlucky as to have been exposed to so much badly written C++ over the years? The checks that I have added and would like to continue to add are primarily focused on "cleaning up the mess" that I've seen in so many C++ code bases over the years. I have seen many uses of unnecessary casting over the years, as opposed to misplaced casts that were intended to prevent loss of precision.

Without access to the original intent (locked up in someone's brain somewhere), whether or not a loss of precision is a problem can only be determined by dynamic analysis or some other form of runtime testing.

Perhaps because both of our interpretations are valid but different points of view, it is best that this check not offer any fixit at all. If it does offer a fixit, then there should be a configuration option to say which kind of fixit to prefer. A developer can then use a workflow like this:

1. Run the tool with no fixits to see if there are any potential problems.
2. Examine each instance and decide if the cast is redundant or misplaced.
3. For redundant casts, run the tool again configured to drop the redundant cast and selectively apply the fixits on the appropriate instances.
4. For misplaced casts, run the tool again configured to move the cast and selectively apply the fixits on the appropriate instances.

This leaves the tool operating conservatively by default, yet still providing the developer with the means of deciding the intent on each flagged instance and applying the appropriate fix for each case.

[danielmarjamaki]

docs/clang-tidy/checks/misc-long-cast.rst

11 ↗

(On Diff #45378)

The use of two colons is intended. When I look at the output here: http://rst.ninjs.org/ it will say "System Message: WARNING/2 (<string>, line 15)" if I use a single colon. If you have a different tool to look at rst that I should use let me know.

[danielmarjamaki]

Fixed review comments

[danielmarjamaki]

clang-tidy/misc/LongCastCheck.cpp

97 ↗

(On Diff #45378)

It does feel strange to me. But I think you are technically right. I can implement it and see what warnings that generates.

[danielmarjamaki]

For information I have now tested latest patch.

Statistics:
projects: 1804
files: 85976
warnings: 100

There were some new interesting warnings and imho they were TP.

[LegalizeAdulthood]

In D16310#335844, @danielmarjamaki wrote:

There were some new interesting warnings and imho they were TP.

TP?

[danielmarjamaki]

Refactoring the AST matching

[danielmarjamaki]

In D16310#337563, @LegalizeAdulthood wrote:

In D16310#335844, @danielmarjamaki wrote:

There were some new interesting warnings and imho they were TP.

TP?

Sorry.. True Positive

clang-tidy/misc/MisplacedWideningCastCheck.cpp

32 ↗

(On Diff #45854)

I would like to use a anyOf(cStyleCastExpr(..), cxxStaticCastExpr(..), ..) ... would that be possible somehow?

I would also like to use anyOf(binaryOperator(..), unaryOperator(..)) is that possible?

[danielmarjamaki]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

33 ↗

(On Diff #46235)

I have refactored these expressions in latest patch. I did not know I have to use stmt().

[danielmarjamaki]

ping?

[alexfh]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

20 ↗

(On Diff #46235)

It makes sense to use the closest common parent, which is expr here, not stmt.

27 ↗

(On Diff #46235)

You can use explicitCastExpr instead of stmt and restructure the matcher to reduce code duplication:

explicitCastExpr(anyOf(cStyleCastExpr(), cxxStaticCastExpr(),
                       cxxReinterpretCastExpr()),
                 has(Calc))

31 ↗

(On Diff #46235)

FYI, these matchers can be combined using anyOf. Not sure whether this will be better (readability-wise and performance-wise) or not.

78 ↗

(On Diff #46235)

IIUC, both "cast" and "calc" are bound in non-optional branches of the matcher, so Cast and Calc should never be nullptr. Please move the !Cast and !Calc checks from ifs to asserts.

88 ↗

(On Diff #46235)

You should be able to check this in the matcher (using hasType(isInteger()) for the expr matcher and a bit more specific hasDestinationType(isInteger()) for explicitCastExpr).

clang-tidy/misc/MisplacedWideningCastCheck.h

16 ↗

(On Diff #46235)

Please add namespace misc {. BTW, do you know the add_new_check.py script that uses the currently recommended template?

21 ↗

(On Diff #46235)

Please add a link to the user docs:

// For the user-facing documentation see:
// http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-widening-cast.html

(and next time use add_new_check.py ;)

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

1 ↗

(On Diff #46235)

If you wonder how to test the RST you write:

1. when running cmake, specify -DCLANG_TOOLS_EXTRA_INCLUDE_DOCS=ON (you can also turn on LLVM_INCLUDE_DOCS and CLANG_INCLUDE_DOCS, if you wish, but this shouldn't be necessary)
2. make/ninja docs-clang-tools-html, the results should be generated in "<your-build-directory>/tools/clang/tools/extra/docs/html"

4 ↗

(On Diff #46235)

nit: Please make the underline the same length as the previous line.

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

31 ↗

(On Diff #46235)

Please use proper punctuation and capitalization in the comments.

[danielmarjamaki]

clang-tidy/misc/MisplacedWideningCastCheck.cpp

31 ↗

(On Diff #46235)

hmm.. are you sure .. for instance this does not work: stmt(anyOf(returnStmt(), varDecl()))

clang-tidy/misc/MisplacedWideningCastCheck.h

16 ↗

(On Diff #46235)

yes I used add_new_check.py to create this checker, thank you.. so I did not think I would have to run it again. I'll try to rerun add_new_check.py in the future.

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

1 ↗

(On Diff #46235)

thanks.. something does not work.

mkdir ~/llvm/build && cd ~/llvm/build
cmake -DCLANG_TOOLS_EXTRA_INCLUDE_DOCS=ON ..
make -j4
make docs-clang-tools-html

>

make: *** No rule to create target ”docs-clang-tools-html”.  Stopping.

I have debian on my computer.

[danielmarjamaki]

Fixes according to review comments.

[danielmarjamaki]

Fixed review comments

clang-tidy/misc/MisplacedWideningCastCheck.cpp

79 ↗

(On Diff #47181)

I looked in another checker , that did not have a condition nor assert so I removed it.

clang-tidy/misc/MisplacedWideningCastCheck.h

17 ↗

(On Diff #47181)

I have rerun add_new_check.py for my latest patch.

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

5 ↗

(On Diff #47181)

yes good catch.

[danielmarjamaki]

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

2 ↗

(On Diff #47182)

For information, it seem I had to use -DLLVM_ENABLE_SPHINX=ON also ..

[alexfh]

Looks good after fixing capitalization in comments.

Thank you!

clang-tidy/misc/MisplacedWideningCastCheck.cpp

32 ↗

(On Diff #47182)

Err, no. I was wrong. Not all of them can be combined: the stmts and decls still need to be separate, so just ignore that comment ;)

80 ↗

(On Diff #47182)

The assert here could simplify debugging a bit in case the matchers are changed and the invariant doesn't hold any more (and the check starts crashing after this line). I don't feel strongly about the presence of an assert in similar places, but there definitely should be no if.

docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

2 ↗

(On Diff #47182)

Yes, thanks. Apparently, I haven't found all relevant flags in ccmake.

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

32 ↗

(On Diff #47182)

You missed the "capitalization" part ;)

[LegalizeAdulthood]

clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp

21–23

Sorry for the late observation, but why doesn't this check for % and / operators?

[danielmarjamaki]

clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp

21–23

That is intentional.. you can't get overflow with / , % , & , | , etc...

clang-tools-extra/trunk/clang-tidy/misc/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyMiscModule
   ArgumentCommentCheck.cpp
   AssertSideEffectCheck.cpp
   AssignOperatorSignatureCheck.cpp
   BoolPointerImplicitConversionCheck.cpp
   DefinitionsInHeadersCheck.cpp
   InaccurateEraseCheck.cpp
   IncorrectRoundings.cpp
   InefficientAlgorithmCheck.cpp
   MacroParenthesesCheck.cpp
   MacroRepeatedSideEffectsCheck.cpp
   MiscTidyModule.cpp
+  MisplacedWideningCastCheck.cpp
   MoveConstantArgumentCheck.cpp
   MoveConstructorInitCheck.cpp
   NewDeleteOverloadsCheck.cpp
   NoexceptMoveConstructorCheck.cpp
   NonCopyableObjects.cpp
   SizeofContainerCheck.cpp
   StaticAssertCheck.cpp
   StringIntegerAssignmentCheck.cpp

clang-tools-extra/trunk/clang-tidy/misc/MiscTidyModule.cpp

 #include "AssignOperatorSignatureCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "DefinitionsInHeadersCheck.h"
 #include "InaccurateEraseCheck.h"
 #include "IncorrectRoundings.h"
 #include "InefficientAlgorithmCheck.h"
 #include "MacroParenthesesCheck.h"
 #include "MacroRepeatedSideEffectsCheck.h"
+#include "MisplacedWideningCastCheck.h"
 #include "MoveConstantArgumentCheck.h"
 #include "MoveConstructorInitCheck.h"
 #include "NewDeleteOverloadsCheck.h"
 #include "NoexceptMoveConstructorCheck.h"
 #include "NonCopyableObjects.h"
 #include "SizeofContainerCheck.h"
 #include "StaticAssertCheck.h"
 #include "StringIntegerAssignmentCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<IncorrectRoundings>(
         "misc-incorrect-roundings");
     CheckFactories.registerCheck<InefficientAlgorithmCheck>(
         "misc-inefficient-algorithm");
     CheckFactories.registerCheck<MacroParenthesesCheck>(
         "misc-macro-parentheses");
     CheckFactories.registerCheck<MacroRepeatedSideEffectsCheck>(
         "misc-macro-repeated-side-effects");
+    CheckFactories.registerCheck<MisplacedWideningCastCheck>(
+        "misc-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveConstantArgumentCheck>(
         "misc-move-const-arg");
     CheckFactories.registerCheck<MoveConstructorInitCheck>(
         "misc-move-constructor-init");
     CheckFactories.registerCheck<NewDeleteOverloadsCheck>(
         "misc-new-delete-overloads");
     CheckFactories.registerCheck<NoexceptMoveConstructorCheck>(
         "misc-noexcept-move-constructor");

clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.h

+//===--- MisplacedWideningCastCheck.h - clang-tidy---------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_WIDENING_CAST_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MISC_MISPLACED_WIDENING_CAST_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+/// Find explicit redundant casts of calculation results to bigger type.
+/// Typically from int to long. If the intention of the cast is to avoid loss
+/// of precision then the cast is misplaced, and there can be loss of
+/// precision. Otherwise such cast is ineffective.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/misc-misplaced-widening-cast.html
+class MisplacedWideningCastCheck : public ClangTidyCheck {
+public:
+  MisplacedWideningCastCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang
+
+#endif

clang-tools-extra/trunk/clang-tidy/misc/MisplacedWideningCastCheck.cpp

+//===--- MisplacedWideningCastCheck.cpp - clang-tidy-----------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#include "MisplacedWideningCastCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace misc {
+
+void MisplacedWideningCastCheck::registerMatchers(MatchFinder *Finder) {
+  auto Calc = expr(anyOf(binaryOperator(anyOf(
+                             hasOperatorName("+"), hasOperatorName("-"),
+                             hasOperatorName("*"), hasOperatorName("<<"))),
+                         unaryOperator(hasOperatorName("~"))),
+                   hasType(isInteger()))
+                  .bind("Calc");
+
+  auto Cast = explicitCastExpr(anyOf(cStyleCastExpr(), cxxStaticCastExpr(),
+                                     cxxReinterpretCastExpr()),
+                               hasDestinationType(isInteger()),
+                               has(Calc))
+                  .bind("Cast");
+
+  Finder->addMatcher(varDecl(has(Cast)), this);
+  Finder->addMatcher(returnStmt(has(Cast)), this);
+  Finder->addMatcher(binaryOperator(hasOperatorName("="), hasRHS(Cast)), this);
+}
+
+static unsigned getMaxCalculationWidth(ASTContext &Context, const Expr *E) {
+  E = E->IgnoreParenImpCasts();
+
+  if (const auto *Bop = dyn_cast<BinaryOperator>(E)) {
+    unsigned LHSWidth = getMaxCalculationWidth(Context, Bop->getLHS());
+    unsigned RHSWidth = getMaxCalculationWidth(Context, Bop->getRHS());
+    if (Bop->getOpcode() == BO_Mul)
+      return LHSWidth + RHSWidth;
+    if (Bop->getOpcode() == BO_Add)
+      return std::max(LHSWidth, RHSWidth) + 1;
+    if (Bop->getOpcode() == BO_Rem) {
+      llvm::APSInt Val;
+      if (Bop->getRHS()->EvaluateAsInt(Val, Context))
+        return Val.getActiveBits();
+    } else if (Bop->getOpcode() == BO_Shl) {
+      llvm::APSInt Bits;
+      if (Bop->getRHS()->EvaluateAsInt(Bits, Context)) {
+        // We don't handle negative values and large values well. It is assumed
+        // that compiler warnings are written for such values so the user will
+        // fix that.
+        return LHSWidth + Bits.getExtValue();
+      }
+
+      // Unknown bitcount, assume there is truncation.
+      return 1024U;
+    }
+  } else if (const auto *Uop = dyn_cast<UnaryOperator>(E)) {
+    // There is truncation when ~ is used.
+    if (Uop->getOpcode() == UO_Not)
+      return 1024U;
+
+    QualType T = Uop->getType();
+    return T->isIntegerType() ? Context.getIntWidth(T) : 1024U;
+  } else if (const auto *I = dyn_cast<IntegerLiteral>(E)) {
+    return I->getValue().getActiveBits();
+  }
+
+  return Context.getIntWidth(E->getType());
+}
+
+void MisplacedWideningCastCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *Cast = Result.Nodes.getNodeAs<ExplicitCastExpr>("Cast");
+  if (Cast->getLocStart().isMacroID())
+    return;
+
+  const auto *Calc = Result.Nodes.getNodeAs<Expr>("Calc");
+  if (Calc->getLocStart().isMacroID())
+    return;
+
+  ASTContext &Context = *Result.Context;
+
+  QualType CastType = Cast->getType();
+  QualType CalcType = Calc->getType();
+
+  if (Context.getIntWidth(CastType) <= Context.getIntWidth(CalcType))
+    return;
+
+  if (Context.getIntWidth(CalcType) >= getMaxCalculationWidth(Context, Calc))
+    return;
+
+  diag(Cast->getLocStart(), "either cast from %0 to %1 is ineffective, or "
+                            "there is loss of precision before the conversion")
+      << CalcType << CastType;
+}
+
+} // namespace misc
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst

.. toctree::
    misc-assign-operator-signature
    misc-bool-pointer-implicit-conversion
    misc-definitions-in-headers
    misc-inaccurate-erase
    misc-incorrect-roundings
    misc-inefficient-algorithm
    misc-macro-parentheses
    misc-macro-repeated-side-effects
+   misc-misplaced-widening-cast
    misc-move-constructor-init
    misc-new-delete-overloads
    misc-noexcept-move-constructor
    misc-non-copyable-objects
    misc-sizeof-container
    misc-static-assert
    misc-string-integer-assignment
    misc-swapped-arguments

clang-tools-extra/trunk/docs/clang-tidy/checks/misc-misplaced-widening-cast.rst

+.. title:: clang-tidy - misc-misplaced-widening-cast
+
+misc-misplaced-widening-cast
+============================
+
+This check will warn when there is a explicit redundant cast of a calculation
+result to a bigger type. If the intention of the cast is to avoid loss of
+precision then the cast is misplaced, and there can be loss of precision.
+Otherwise the cast is ineffective.
+
+Example code::
+
+    long f(int x) {
+        return (long)(x*1000);
+    }
+
+The result x*1000 is first calculated using int precision. If the result
+exceeds int precision there is loss of precision. Then the result is casted to
+long.
+
+If there is no loss of precision then the cast can be removed or you can
+explicitly cast to int instead.
+
+If you want to avoid loss of precision then put the cast in a proper location,
+for instance::
+
+    long f(int x) {
+        return (long)x * 1000;
+    }
+
+Floating point
+--------------
+
+Currently warnings are only written for integer conversion. No warning is
+written for this code::
+
+    double f(float x) {
+        return (double)(x * 10.0f);
+    }

clang-tools-extra/trunk/test/clang-tidy/misc-misplaced-widening-cast.cpp

+// RUN: %check_clang_tidy %s misc-misplaced-widening-cast %t
+
+void assign(int a, int b) {
+  long l;
+
+  l = a * b;
+  l = (long)(a * b);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
+  l = (long)a * b;
+
+  l = (long)(a << 8);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long'
+  l = (long)b << 8;
+
+  l = static_cast<long>(a * b);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: either cast from 'int' to 'long' is ineffective, or there is loss of precision before the conversion [misc-misplaced-widening-cast]
+}
+
+void init(unsigned int n) {
+  long l = (long)(n << 8);
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: either cast from 'unsigned int'
+}
+
+long ret(int a) {
+  return (long)(a * 1000);
+  // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: either cast from 'int' to 'long'
+}
+
+void dontwarn1(unsigned char a, int i, unsigned char *p) {
+  long l;
+  // The result is a 9 bit value, there is no truncation in the implicit cast.
+  l = (long)(a + 15);
+  // The result is a 12 bit value, there is no truncation in the implicit cast.
+  l = (long)(a << 4);
+  // The result is a 3 bit value, there is no truncation in the implicit cast.
+  l = (long)((i%5)+1);
+  // The result is a 16 bit value, there is no truncation in the implicit cast.
+  l = (long)(((*p)<<8) + *(p+1));
+}
+
+template <class T> struct DontWarn2 {
+  void assign(T a, T b) {
+    long l;
+    l = (long)(a * b);
+  }
+};
+DontWarn2<int> DW2;
+
+// Cast is not suspicious when casting macro.
+#define A  (X<<2)
+long macro1(int X) {
+  return (long)A;
+}
+
+// Don't warn about cast in macro.
+#define B(X,Y)   (long)(X*Y)
+long macro2(int x, int y) {
+  return B(x,y);
+}
+
+void floatingpoint(float a, float b) {
+  double d = (double)(a * b); // Currently we don't warn for this.
+}