[clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[courbet]

Summary

Checks for narrowing conversions, e.g.

int i = 0;
i += 0.1;

[alexfh]

Before going deeper, I'd like to understand what's the difference between this check and -Wconversion group of clang warnings?

[courbet]

This check is a more useful subset of -Wfloat-conversion that targets cases that are more likely to be bugs than the rest of -Wfloat-conversion.

Since "more useful" is a somewhat subjective notion, let me point out a typical class of bugs that this uncovers:

// Paying 0.5 less dollars per day.
int to_be_payed = 0;
for (const auto& v : us_dollars_per_day) {
  to_be_payed += v[i];
}

This is not necessarily more dangerous than int my_int = my_float;, but the effects will add up dramatically.

Why can't this be done in clang ?

It could, but we would need to add more fine-grained control of what to enable or not enable. It feels more inline with clang-tidy's options approach, but I don;t have strong feelings on this.

[JonasToth]

One more reason for this check being in clang-tidy would be automatically inserting narrow_cast<T>(v), which is the utility of the gsl to make narrowing casts explicitly visible.
Doing might be the result of future work.

clang-tidy implements bugprone-integer-division already, which does a somewhat related thing:
"Finds cases where integer division in a floating point context is likely to cause unintended loss of precision." - problematic narrowing conversion.

[courbet]

In D38455#886141, @JonasToth wrote:

One more reason for this check being in clang-tidy would be automatically inserting narrow_cast<T>(v), which is the utility of the gsl to make narrowing casts explicitly visible.
Doing might be the result of future work.

Do we already have other cppcoreguidelines checks that provides fixes with symbols from the the gsl ?

[JonasToth]

Do we already have other cppcoreguidelines checks that provides fixes with symbols from the the gsl ?

AFAIK not yet. I am working on fixits for gsl::owner<> and the bounds-* stuff could suggest gsl::at() as well.

I think you should add std::floor and std::ceil recognition here. This would be inline with hicpp, too.

[alexfh]

Now I finally got around to this.

Since the "Enforcement" part of the relevant C++ Core Guidelines rule is not very helpful (kind of a "hey, we know enforcing this rule will lead to tons of false positive, so we don't know what exactly makes sense to enforce"), this may well be a good first step. I personally have no concerns. Maybe someone more interested in this module will have something to say though.

Could you rebase the patch against current head?

[alexfh]

In D38455#887142, @JonasToth wrote:

I think you should add std::floor and std::ceil recognition here.

+1

[courbet]

Do not trigger on some_int += std::floor(some_float);

[aaron.ballman]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

26 ↗

(On Diff #138910)

Why only these two operators? This does not match the behavior from the C++ Core Guideline check itself (https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing).

[courbet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

26 ↗

(On Diff #138910)

These provided the best signal to noise ratio. Also they are the most dangerous (in a loop, you might end up losing one unit per iteration). I'll add other operators later if that's fine with you.

[aaron.ballman]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

26 ↗

(On Diff #138910)

If the check doesn't cover anything listed in the C++ Core Guideline examples or enforcement wording, I have a hard time accepting it as the initial commit for such a check.

Of special interest to me is the request from the C++ Core Guideline authors themselves:

- flag all floating-point to integer conversions (maybe only float->char and double->int. Here be dragons! we need data)
- flag all long->char (I suspect int->char is very common. Here be dragons! we need data)

I think we need data as well -- writing the check and then testing it over several million lines of source could give us some of that data, which we can then feed back to the guideline authors, so that we come up with a check that's realistic.

[courbet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

26 ↗

(On Diff #138910)

Quoting the guideline:
"A good analyzer can detect all narrowing conversions. However, flagging all narrowing conversions will lead to a lot of false positives." Then follows a list of suggestions. While +=/-= are not in the list of suggestions, they are a subset of "flag all floating-point to integer conversions" that I've found to be useful (very low rate of false positives) by running on our codebase.
I agree that more data would be useful, so I'll do an analysis of flagging all (non-ceil/floor float/double)->integral conversions.

[alexfh]

I agree that more data would be useful, so I'll do an analysis of flagging all (non-ceil/floor float/double)->integral conversions.

Removing from my dashboard for now.

[courbet]

OK, here's an analysis of 100 instances of a check that would warn on all fully implicit (no C/static/reinterpret/...) casts from float/double to integral: link
I don't expect the analysis to be perfect, but that gives us a better idea of what to expect.

• A bit more than 1/3 of instances are OK and should be using a cast.
• A bit less than 1/4 of instances is brittle code and should use int types.
• The two false positive cases are easy to handle.
• I was surprised about the "truth value of float" one. I think we should provide a fix to turn !f this into f != 0.0f.

[courbet]

ping

[JonasToth]

Hi,

my 2 cents:

• On which codebases did you run the check?
• did you consider looking for implicitCastExpr? You can capture all narrowing conversion with that and analyze them further. I think it is possible to warn for the subset mentioned in the guidelines.
• you match for binaryOperator("+=", "-") maybe all assignments can be looked at? (binaryOperator(isASsignmentOperator()), defined in clang-tidy/util/Matchers.h or similar) That includes all calculate-and-assign operations. Those should be equally dangerous.

[courbet]

Hi Jonas,

In D38455#1061228, @JonasToth wrote:

Hi,

my 2 cents:

• On which codebases did you run the check?

A large repository of open-source code, plus internal code at google. External code includes e.g. code from ffmpeg, Eigen, R, Chromium, gnuplot, lua ,...

• did you consider looking for implicitCastExpr? You can capture all narrowing conversion with that and analyze them further. I think it is possible to warn for the subset mentioned in the guidelines.

Yes, that's the version for which I have provided analysis. I'll update the diff with that version.

• you match for binaryOperator("+=", "-") maybe all assignments can be looked at? (binaryOperator(isASsignmentOperator()), defined in clang-tidy/util/Matchers.h or similar) That includes all calculate-and-assign operations. Those should be equally dangerous.

The "normal" assignments are covered by the implicitCastExpr() above.

[JonasToth]

That sounds good.

Removing from my dashboard for now.

@aaron.ballman seems to be busy now, maybe you should add alexfh again and discuss the results.

[courbet]

• Add support for bad cast detection.

[courbet]

In D38455#1061300, @JonasToth wrote:

That sounds good.

Removing from my dashboard for now.

maybe you should add alexfh again and discuss the results.

Is there anything I need to do for the diff to change state ? I thought updating the code would automatically mark it "ready for review" again.

[JonasToth]

Is there anything I need to do for the diff to change state ? I

thought updating the code would automatically mark it "ready for review"
again.

I think all comments must be marked as done to be ready for review
again. Usually the reviewer reacts to changed code, too.

But aaron seems busy right now and i think alexfh did disable the
notifications for now. Add/ping him again.

Am 09.04.2018 um 12:55 schrieb Clement Courbet via Phabricator:

courbet added a comment.

In https://reviews.llvm.org/D38455#1061300, @JonasToth wrote:

That sounds good.

Removing from my dashboard for now.

maybe you should add alexfh again and discuss the results.

Is there anything I need to do for the diff to change state ? I thought updating the code would automatically mark it "ready for review" again.

Repository:

rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

[courbet]

In D38455#1061406, @JonasToth wrote:

I think all comments must be marked as done to be ready for review
again.

I think alexfh did disable the notifications for now. Add/ping him again.

I see, thanks.

[JonasToth]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

32 ↗

(On Diff #141610)

Do you plan to check for double -> float conversion, too?

Having a limited scope for now is ok, but a FIXME would be nice.

34 ↗

(On Diff #141610)

Given C++ is weird sometimes: Is there a way that a cast might imply an narrowing conversion?

double value = 0.4;
int i = static_cast<float>(value);

or

void some_function(int);

double d = 0.42;
some_function(static_cast<float>(d));

would come to mind.

Nice to have, IMHO not necessary.

41 ↗

(On Diff #141610)

I would really like to add all other calc-and-assign operations. At least "*=" and "/=".

[courbet]

Thanks.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

32 ↗

(On Diff #141610)

I've added a FIXME.

34 ↗

(On Diff #141610)

Luckily that does not seem to be implicit:

void f(double value) {
  int i = static_cast<float>(value);
}

FunctionDecl 0x7fe5ffbd4150 <experimental/users/courbet/benchmark/toto.cc:1:1, line:3:1> line:1:6 f 'void (double)'
|-ParmVarDecl 0x7fe5ffbd4088 <col:8, col:15> col:15 used value 'double'
`-CompoundStmt 0x7fe5ffbd4380 <col:22, line:3:1>
  `-DeclStmt 0x7fe5ffbd4368 <line:2:3, col:36>
    `-VarDecl 0x7fe5ffbd4250 <col:3, col:35> col:7 i 'int' cinit
      `-ImplicitCastExpr 0x7fe5ffbd4350 <col:11, col:35> 'int' <FloatingToIntegral>
        `-CXXStaticCastExpr 0x7fe5ffbd4320 <col:11, col:35> 'float' static_cast<float> <NoOp>
          `-ImplicitCastExpr 0x7fe5ffbd4308 <col:30> 'float' <FloatingCast>
            `-ImplicitCastExpr 0x7fe5ffbd42f0 <col:30> 'double' <LValueToRValue>
              `-DeclRefExpr 0x7fe5ffbd42b0 <col:30> 'double' lvalue ParmVar 0x7fe5ffbd4088 'value' 'double'

41 ↗

(On Diff #141610)

Makes sense, I've added *= and /=. All others (%=, &=, <<= and variations thereof) trigger a compilation error if the RHS is a float (rightfully).

[courbet]

• Add *= and /=.

[JonasToth]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

34 ↗

(On Diff #141610)

The interesting one should get diagnosed. Could you add a test?

41 ↗

(On Diff #141610)

For floats that is true.

If we care about the ìnt->char casts later, they should be added.

[JonasToth]

Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too.

[alexfh]

In D38455#1061195, @courbet wrote:

ping

Sorry for the long review due to the holidays.

Generally, I would also like Aaron to take a look when he's back, since he had some concerns. While we're waiting, one minor comment from me.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

41 ↗

(On Diff #141610)

Does it make sense to match all assignment operators including just =? If so, the matcher will become a bit simpler: binaryOperator(isAssignmentOperator(), ...). If there's a reason not to do this, maybe adding a comment would be useful

Apart from that, I wonder whether the matcher above would also cover the case of assignment operators? I would expect the AST for assignment expressions with different types to have ImplicitCast nodes anyway.

[courbet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

41 ↗

(On Diff #141610)

If we care about the ìnt->char casts later, they should be added.

Will do. I'd like to start by implementing only floats first though, because they are the ones where I have data.

Apart from that, I wonder whether the matcher above would also cover the case of assignment operators?

It does cover = (which generates an implicit cast), but not +=.

[pfultz2]

This seems like it would be nice to have in bugprone-* as well.

[courbet]

In D38455#1061681, @JonasToth wrote:

Could you please add some tests that include user defined literals and there interaction with other literals. We should catch narrowing conversions from them, too.

User defined literals do not have this issue: the only accepted signatures are with long double or unsigned long long parameters. Narrowing cannot happen because XYZ_ud actually means operator""(XYZull) (same for floats).
(see http://en.cppreference.com/w/cpp/language/user_literal).

I've added a test with a narrowing on the return value.

[courbet]

• Simplify matcher expression
• Add other binary operators
• Add more tests.

[courbet]

In D38455#1061926, @pfultz2 wrote:

This seems like it would be nice to have in bugprone-* as well.

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

[JonasToth]

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

[courbet]

• Add NarrowingConversionsCheck to bugprone module.

[courbet]

In D38455#1062718, @JonasToth wrote:

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

Thanks for the pointer !

[alexfh]

In D38455#1062722, @courbet wrote:

In D38455#1062718, @JonasToth wrote:

Sure. Is it OK to add a dependency from clang-tidy/bugprone/BugproneTidyModule.cpp to stuff in clang-tidy/cpp-coreguidelines ? Is there an existing example of this ?

Take a look in the hicpp module, almost everything there is aliased :)

Thanks for the pointer !

We currently don't have any strict rules about aliases, but we should remember that aliases have certain costs (in terms of maintenance and possible user-facing issues like duplicate warnings from different checks). I think that in general it would be better to avoid excessive aliases. There are cases when aliases are inevitable (or at least feel like a proper solution), like when multiple style guides have the same rule or very similar rules that make sense to be implemented in a single check with a bit of configurability to easily accommodate the differences. But in this case there's so far one specific rule of the C++ Core Guidelines that the check is enforcing, and adding an alias under bugprone- for it seems to be unnecessary. I also think that if a user wants to enable this check, they'd better know that it backs up a rule of the C++ Core Guidelines rather than being just an invention of clang-tidy contributors (there's nothing wrong with the latter, it's just nice to give credits where appropriate ;).

I suggest to try applying the following decision process to find proper place(s) for a check:

1. List all use cases we want to support, decide whether customizations (via check options) will be needed to support them. Use case may be
   • enforcement of a rule of a certain style guide a clang-tidy module for which exists or is being added together with the check;
   • generic use case not covered by a rule of a style guide. These only count if there is no "style guide" use case that doesn't require customizations.

2. If the check only has a single "style guide" use case, place it to the corresponding module.
3. If a check (without customizations) implements rules of two or more style guides, and dependency structure allows, place it to one of the corresponding modules and add aliases to the others.
4. Otherwise choose a "generic" module for the check (bugprone-, readability-, modernize-, performance-, portability-, or misc-, if the check doesn't suit a more specific module). Add aliases to the "style guide" modules, if needed.

[courbet]

@aaron.ballman Ping

[aaron.ballman]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

35 ↗

(On Diff #141801)

I believe this code will not diagnose under this check -- is that intended as a way to silence the check?

i += (double)0.5;

docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst

10–11 ↗

(On Diff #141801)

More exposition is needed because this is only a very small part of the core guideline. You should be explicit about where we deviate (or what we support, depending on which is the clearest exposition).

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

81 ↗

(On Diff #141801)

What should happen in cases like the following:

template <typename T1, typename T2>
void f(T1 one, T2 two) {
  one += two;
}

void g() {
  f(1, 2);
  f(1, .5);
}

#define DERP(i, j) (i += j)

void h() {
  int i = 0;
  DERP(1, 2);
  DERP(i, .5);
}

[courbet]

• More explicit documentation.
• Do not trigger in template and macro contexts.
• More tests.

[courbet]

Thanks.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

35 ↗

(On Diff #141801)

Did you mean (int)0.5 ?

Yes, the user essentially told us they knew what they were doing. I've added an explicit test for this.

test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

81 ↗

(On Diff #141801)

I added more tests.

[aaron.ballman]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

35 ↗

(On Diff #141801)

I truly meant (double)0.5 -- where the cast has no impact on the narrowing conversion, but the check still doesn't diagnose because there's an explicit cast present. Should the check be checking for explicit casts to the narrowed type?

[courbet]

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

35 ↗

(On Diff #141801)

OK, then that's fine (I added a test for that for): there is an explicit cast to double, but then the compiler generates an extra cast to int on top, which triggers.

[aaron.ballman]

I think this generally LGTM. You should wait a bit to see if @alexfh has any other concerns.

clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

35 ↗

(On Diff #141801)

Ah, excellent, thank you!

[courbet]

Great, thanks for the review !

clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp

 //===--- BugproneTidyModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../cppcoreguidelines/NarrowingConversionsCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
 #include "CopyConstructorInitCheck.h"
 #include "DanglingHandleCheck.h"
 #include "FoldInitTypeCheck.h"
 #include "ForwardDeclarationNamespaceCheck.h"
 #include "ForwardingReferenceOverloadCheck.h"
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<MisplacedOperatorInStrlenInAllocCheck>(
         "bugprone-misplaced-operator-in-strlen-in-alloc");
     CheckFactories.registerCheck<MisplacedWideningCastCheck>(
         "bugprone-misplaced-widening-cast");
     CheckFactories.registerCheck<MoveForwardingReferenceCheck>(
         "bugprone-move-forwarding-reference");
     CheckFactories.registerCheck<MultipleStatementMacroCheck>(
         "bugprone-multiple-statement-macro");
+    CheckFactories.registerCheck<cppcoreguidelines::NarrowingConversionsCheck>(
+        "bugprone-narrowing-conversions");
     CheckFactories.registerCheck<ParentVirtualCallCheck>(
         "bugprone-parent-virtual-call");
     CheckFactories.registerCheck<SizeofContainerCheck>(
         "bugprone-sizeof-container");
     CheckFactories.registerCheck<SizeofExpressionCheck>(
         "bugprone-sizeof-expression");
     CheckFactories.registerCheck<StringConstructorCheck>(
         "bugprone-string-constructor");

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

add_clang_library(clangTidyBugproneModule
 
   LINK_LIBS
   clangAnalysis
   clangAST
   clangASTMatchers
   clangBasic
   clangLex
   clangTidy
+  clangTidyCppCoreGuidelinesModule
   clangTidyUtils
   clangTooling
   )

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

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyCppCoreGuidelinesModule
   AvoidGotoCheck.cpp
   CppCoreGuidelinesTidyModule.cpp
   InterfacesGlobalInitCheck.cpp
+  NarrowingConversionsCheck.cpp
   NoMallocCheck.cpp
   OwningMemoryCheck.cpp
   ProBoundsArrayToPointerDecayCheck.cpp
   ProBoundsConstantArrayIndexCheck.cpp
   ProBoundsPointerArithmeticCheck.cpp
   ProTypeConstCastCheck.cpp
   ProTypeCstyleCastCheck.cpp
   ProTypeMemberInitCheck.cpp

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp

 //===--- CppCoreGuidelinesModule.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 "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
 #include "../misc/UnconventionalAssignOperatorCheck.h"
 #include "AvoidGotoCheck.h"
 #include "InterfacesGlobalInitCheck.h"
+#include "NarrowingConversionsCheck.h"
 #include "NoMallocCheck.h"
 #include "OwningMemoryCheck.h"
 #include "ProBoundsArrayToPointerDecayCheck.h"
 #include "ProBoundsConstantArrayIndexCheck.h"
 #include "ProBoundsPointerArithmeticCheck.h"
 #include "ProTypeConstCastCheck.h"
 #include "ProTypeCstyleCastCheck.h"
 #include "ProTypeMemberInitCheck.h"

 /// A module containing checks of the C++ Core Guidelines
 class CppCoreGuidelinesModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidGotoCheck>(
         "cppcoreguidelines-avoid-goto");
     CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
         "cppcoreguidelines-interfaces-global-init");
+    CheckFactories.registerCheck<NarrowingConversionsCheck>(
+        "cppcoreguidelines-narrowing-conversions");
     CheckFactories.registerCheck<NoMallocCheck>("cppcoreguidelines-no-malloc");
     CheckFactories.registerCheck<OwningMemoryCheck>(
         "cppcoreguidelines-owning-memory");
     CheckFactories.registerCheck<ProBoundsArrayToPointerDecayCheck>(
         "cppcoreguidelines-pro-bounds-array-to-pointer-decay");
     CheckFactories.registerCheck<ProBoundsConstantArrayIndexCheck>(
         "cppcoreguidelines-pro-bounds-constant-array-index");
     CheckFactories.registerCheck<ProBoundsPointerArithmeticCheck>(

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h

+//===--- NarrowingConversionsCheck.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_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+/// Checks for narrowing conversions, e.g:
+///   int i = 0;
+///   i += 0.1;
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.html
+class NarrowingConversionsCheck : public ClangTidyCheck {
+public:
+  NarrowingConversionsCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_NARROWING_CONVERSIONS_H

clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp

+//===--- NarrowingConversionsCheck.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 "NarrowingConversionsCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace cppcoreguidelines {
+
+// FIXME: Check double -> float truncation. Pay attention to casts:
+void NarrowingConversionsCheck::registerMatchers(MatchFinder *Finder) {
+  // ceil() and floor() are guaranteed to return integers, even though the type
+  // is not integral.
+  const auto IsCeilFloorCall = callExpr(callee(functionDecl(
+      hasAnyName("::ceil", "::std::ceil", "::floor", "::std::floor"))));
+
+  const auto IsFloatExpr =
+      expr(hasType(realFloatingPointType()), unless(IsCeilFloorCall));
+
+  // casts:
+  //   i = 0.5;
+  //   void f(int); f(0.5);
+  Finder->addMatcher(implicitCastExpr(hasImplicitDestinationType(isInteger()),
+                                      hasSourceExpression(IsFloatExpr),
+                                      unless(hasParent(castExpr())),
+                                      unless(isInTemplateInstantiation()))
+                         .bind("cast"),
+                     this);
+
+  // Binary operators:
+  //   i += 0.5;
+  Finder->addMatcher(
+      binaryOperator(isAssignmentOperator(),
+                     // The `=` case generates an implicit cast which is covered
+                     // by the previous matcher.
+                     unless(hasOperatorName("=")),
+                     hasLHS(hasType(isInteger())), hasRHS(IsFloatExpr),
+                     unless(isInTemplateInstantiation()))
+          .bind("op"),
+      this);
+}
+
+void NarrowingConversionsCheck::check(const MatchFinder::MatchResult &Result) {
+  if (const auto *Op = Result.Nodes.getNodeAs<BinaryOperator>("op")) {
+    if (Op->getLocStart().isMacroID())
+      return;
+    diag(Op->getOperatorLoc(), "narrowing conversion from %0 to %1")
+        << Op->getRHS()->getType() << Op->getLHS()->getType();
+    return;
+  }
+  const auto *Cast = Result.Nodes.getNodeAs<ImplicitCastExpr>("cast");
+  if (Cast->getLocStart().isMacroID())
+    return;
+  diag(Cast->getExprLoc(), "narrowing conversion from %0 to %1")
+      << Cast->getSubExpr()->getType() << Cast->getType();
+}
+
+} // namespace cppcoreguidelines
+} // namespace tidy
+} // namespace clang

clang-tools-extra/trunk/docs/ReleaseNotes.rst

 - The 'misc-undelegated-constructor' check was renamed to :doc:`bugprone-undelegated-constructor
   <clang-tidy/checks/bugprone-undelegated-constructor>`
 
 - The 'misc-unused-raii' check was renamed to :doc:`bugprone-unused-raii
   <clang-tidy/checks/bugprone-unused-raii>`
 
 - The 'google-runtime-member-string-references' check was removed.
 
+- New `cppcoreguidelines-narrowing-conversions
+  <http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.html>`_ check
+
+  Checks for narrowing conversions, e.g. ``int i = 0; i += 0.1;``.
+
 Improvements to include-fixer
 -----------------------------
 
 The improvements are...
 
 Improvements to modularize
 --------------------------
 
 The improvements are...

clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst

+.. title:: clang-tidy - cppcoreguidelines-narrowing-conversions
+
+cppcoreguidelines-narrowing-conversions
+=======================================
+
+Checks for silent narrowing conversions, e.g: ``int i = 0; i += 0.1;``. While
+the issue is obvious in this former example, it might not be so in the
+following: ``void MyClass::f(double d) { int_member_ += d; }``.
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, corresponding to rule ES.46. See
+
+https://github.com/isocpp/CppCoreGuidelines/blob/master/CppCoreGuidelines.md#Res-narrowing.
+
+We enforce only part of the guideline, more specifically, we flag:
+ - All floating-point to integer conversions that are not marked by an explicit
+   cast (c-style or ``static_cast``). For example: ``int i = 0; i += 0.1;``,
+   ``void f(int); f(0.1);``,
+ - All applications of binary operators where the left-hand-side is an integer
+   and the right-hand-size is a floating-point. For example:
+   ``int i; i+= 0.1;``.
+

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

.. toctree::
    cert-fio38-c (redirects to misc-non-copyable-objects) <cert-fio38-c>
    cert-flp30-c
    cert-msc30-c (redirects to cert-msc50-cpp) <cert-msc30-c>
    cert-msc50-cpp
    cert-oop11-cpp (redirects to performance-move-constructor-init) <cert-oop11-cpp>
    cppcoreguidelines-avoid-goto
    cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) <cppcoreguidelines-c-copy-assignment-signature>
    cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
    cppcoreguidelines-no-malloc
    cppcoreguidelines-owning-memory
    cppcoreguidelines-pro-bounds-array-to-pointer-decay
    cppcoreguidelines-pro-bounds-constant-array-index
    cppcoreguidelines-pro-bounds-pointer-arithmetic
    cppcoreguidelines-pro-type-const-cast
    cppcoreguidelines-pro-type-cstyle-cast
    cppcoreguidelines-pro-type-member-init

clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+namespace floats {
+
+struct ConvertsToFloat {
+  operator float() const { return 0.5; }
+};
+
+float operator "" _Pa(unsigned long long);
+
+void not_ok(double d) {
+  int i = 0;
+  i = d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i = 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i = static_cast<float>(d);
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i = ConvertsToFloat();
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i = 15_Pa;
+  // CHECK-MESSAGES: :[[@LINE-1]]:7: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void not_ok_binary_ops(double d) {
+  int i = 0;
+  i += 0.5;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += d;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  // We warn on the following even though it's not dangerous because there is no
+  // reason to use a double literal here.
+  // TODO(courbet): Provide an automatic fix.
+  i += 2.0;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += 2.0f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+
+  i *= 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i /= 0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'float' to 'int' [cppcoreguidelines-narrowing-conversions]
+  i += (double)0.5f;
+  // CHECK-MESSAGES: :[[@LINE-1]]:5: warning: narrowing conversion from 'double' to 'int' [cppcoreguidelines-narrowing-conversions]
+}
+
+void ok(double d) {
+  int i = 0;
+  i = 1;
+  i = static_cast<int>(0.5);
+  i = static_cast<int>(d);
+  i = std::ceil(0.5);
+  i = ::std::floor(0.5);
+  {
+    using std::ceil;
+    i = ceil(0.5f);
+  }
+  i = ceil(0.5f);
+}
+
+void ok_binary_ops(double d) {
+  int i = 0;
+  i += 1;
+  i += static_cast<int>(0.5);
+  i += static_cast<int>(d);
+  i += (int)d;
+  i += std::ceil(0.5);
+  i += ::std::floor(0.5);
+  {
+    using std::ceil;
+    i += ceil(0.5f);
+  }
+  i += ceil(0.5f);
+}
+
+// We're bailing out in templates and macros.
+template <typename T1, typename T2>
+void f(T1 one, T2 two) {
+  one += two;
+}
+
+void template_context() {
+  f(1, 2);
+  f(1, .5);
+}
+
+#define DERP(i, j) (i += j)
+
+void macro_context() {
+  int i = 0;
+  DERP(i, 2);
+  DERP(i, .5);
+}
+
+}  // namespace floats