[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL333066: [clang-tidy] new 
cppcoreguidelines-narrowing-conversions check. (authored by courbet, committed 
by ).

Repository:
  rL LLVM

https://reviews.llvm.org/D38455

Files:
  clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
  clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CMakeLists.txt
  
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  
clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  clang-tools-extra/trunk/docs/ReleaseNotes.rst
  
clang-tools-extra/trunk/docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  clang-tools-extra/trunk/docs/clang-tidy/checks/list.rst
  
clang-tools-extra/trunk/test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
+++ clang-tools-extra/trunk/clang-tidy/bugprone/CMakeLists.txt
@@ -48,6 +48,7 @@
   clangBasic
   clangLex
   clangTidy
+  clangTidyCppCoreGuidelinesModule
   clangTidyUtils
   clangTooling
   )
Index: clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
===
--- clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
+++ clang-tools-extra/trunk/clang-tidy/bugprone/BugproneTidyModule.cpp
@@ -10,6 +10,7 @@
 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "../cppcoreguidelines/NarrowingConversionsCheck.h"
 #include "ArgumentCommentCheck.h"
 #include "AssertSideEffectCheck.h"
 #include "BoolPointerImplicitConversionCheck.h"
@@ -92,6 +93,8 @@
 "bugprone-move-forwarding-reference");
 CheckFactories.registerCheck(
 "bugprone-multiple-statement-macro");
+CheckFactories.registerCheck(
+"bugprone-narrowing-conversions");
 CheckFactories.registerCheck(
 "bugprone-parent-virtual-call");
 CheckFactories.registerCheck(
Index: clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
===
--- clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
+++ clang-tools-extra/trunk/clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
@@ -0,0 +1,70 @@
+//===--- 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 ) {
+  if (const auto *Op = Result.Nodes.getNodeAs("op")) {
+if (Op->getLocStart().isMacroID())
+  return;
+diag(Op->getOperatorLoc(), "narrowing conversion from %0 to %1")
+<< Op->getRHS()->getType() << Op->getLHS()->getType();
+return;
+  }
+  const 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Great, thanks for the review !


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.

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




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

courbet wrote:
> aaron.ballman wrote:
> > courbet wrote:
> > > aaron.ballman wrote:
> > > > I believe this code will not diagnose under this check -- is that 
> > > > intended as a way to silence the check?
> > > > ```
> > > > i += (double)0.5;
> > > > ```
> > > 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.
> > 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?
> 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.
Ah, excellent, thank you!


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

aaron.ballman wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > I believe this code will not diagnose under this check -- is that 
> > > intended as a way to silence the check?
> > > ```
> > > i += (double)0.5;
> > > ```
> > 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.
> 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?
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

courbet wrote:
> aaron.ballman wrote:
> > I believe this code will not diagnose under this check -- is that intended 
> > as a way to silence the check?
> > ```
> > i += (double)0.5;
> > ```
> 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.
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?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Thanks.




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

aaron.ballman wrote:
> I believe this code will not diagnose under this check -- is that intended as 
> a way to silence the check?
> ```
> i += (double)0.5;
> ```
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.



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:81
+
+}  // namespace floats

aaron.ballman wrote:
> What should happen in cases like the following:
> ```
> template 
> 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);
> }
> ```
I added more tests.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 147508.
courbet marked 2 inline comments as done.
courbet added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,103 @@
+// 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(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(0.5);
+  i = static_cast(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(0.5);
+  i += static_cast(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 
+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
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -78,6 +78,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:35
+  hasSourceExpression(IsFloatExpr),
+  unless(hasParent(castExpr(
+ .bind("cast"),

I believe this code will not diagnose under this check -- is that intended as a 
way to silence the check?
```
i += (double)0.5;
```



Comment at: 
docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst:10-11
+
+This rule is part of the "Expressions and statements" profile of the C++ Core
+Guidelines, corresponding to rule ES.46. See
+

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



Comment at: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp:81
+
+}  // namespace floats

What should happen in cases like the following:
```
template 
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);
}
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

@aaron.ballman Ping


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/D38455#1062722, @courbet wrote:

> In https://reviews.llvm.org/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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In https://reviews.llvm.org/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 !


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 141801.
courbet added a comment.

- Add NarrowingConversionsCheck to bugprone module.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/bugprone/BugproneTidyModule.cpp
  clang-tidy/bugprone/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,81 @@
+// 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(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]
+}
+
+void ok(double d) {
+  int i = 0;
+  i = 1;
+  i = static_cast(0.5);
+  i = static_cast(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(0.5);
+  i += static_cast(d);
+  i += std::ceil(0.5);
+  i += ::std::floor(0.5);
+  {
+using std::ceil;
+i += ceil(0.5f);
+  }
+  i += ceil(0.5f);
+}
+
+}  // namespace floats
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. 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" 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> 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 :)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 141800.
courbet added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,81 @@
+// 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(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]
+}
+
+void ok(double d) {
+  int i = 0;
+  i = 1;
+  i = static_cast(0.5);
+  i = static_cast(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(0.5);
+  i += static_cast(d);
+  i += std::ceil(0.5);
+  i += ::std::floor(0.5);
+  {
+using std::ceil;
+i += ceil(0.5f);
+  }
+  i += ceil(0.5f);
+}
+
+}  // namespace floats
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. 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 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In https://reviews.llvm.org/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 ?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In https://reviews.llvm.org/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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Paul Fultz II via Phabricator via cfe-commits]

pfultz2 added a comment.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

alexfh wrote:
> JonasToth wrote:
> > courbet wrote:
> > > JonasToth wrote:
> > > > I would really like to add all other calc-and-assign operations. At 
> > > > least "*=" and "/=".
> > > Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and 
> > > variations thereof)  trigger a compilation error if the RHS is a float 
> > > (rightfully).
> > > 
> > > 
> > For floats that is true.
> > 
> > If we care about the `ìnt->char` casts later, they should be added.
> 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.
> 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 `+=`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added a comment.

In https://reviews.llvm.org/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.




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

JonasToth wrote:
> courbet wrote:
> > JonasToth wrote:
> > > I would really like to add all other calc-and-assign operations. At least 
> > > "*=" and "/=".
> > Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and 
> > variations thereof)  trigger a compilation error if the RHS is a float 
> > (rightfully).
> > 
> > 
> For floats that is true.
> 
> If we care about the `ìnt->char` casts later, they should be added.
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34
+   hasSourceExpression(IsFloatExpr),
+   unless(hasParent(castExpr(.bind("cast"),
+  this);

courbet wrote:
> JonasToth wrote:
> > 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(value);
> > ```
> > 
> > or 
> > 
> > ```
> > void some_function(int);
> > 
> > double d = 0.42;
> > some_function(static_cast(d));
> > ```
> > would come to mind.
> > 
> > Nice to have, IMHO not necessary.
> Luckily that does not seem to be implicit:
> 
> ```
> void f(double value) {
>   int i = static_cast(value);
> }
> ```
> 
> ```
> FunctionDecl 0x7fe5ffbd4150 
>  line:1:6 f 'void 
> (double)'
> |-ParmVarDecl 0x7fe5ffbd4088  col:15 used value 'double'
> `-CompoundStmt 0x7fe5ffbd4380 
>   `-DeclStmt 0x7fe5ffbd4368 
> `-VarDecl 0x7fe5ffbd4250  col:7 i 'int' cinit
>   `-ImplicitCastExpr 0x7fe5ffbd4350  'int' 
> 
> `-CXXStaticCastExpr 0x7fe5ffbd4320  'float' 
> static_cast 
>   `-ImplicitCastExpr 0x7fe5ffbd4308  'float' 
> `-ImplicitCastExpr 0x7fe5ffbd42f0  'double' 
> 
>   `-DeclRefExpr 0x7fe5ffbd42b0  'double' lvalue ParmVar 
> 0x7fe5ffbd4088 'value' 'double'
> ```
> 
> 
> 
The interesting one should get diagnosed. Could you add a test?



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

courbet wrote:
> JonasToth wrote:
> > I would really like to add all other calc-and-assign operations. At least 
> > "*=" and "/=".
> Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and 
> variations thereof)  trigger a compilation error if the RHS is a float 
> (rightfully).
> 
> 
For floats that is true.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 141635.
courbet marked 2 inline comments as done.
courbet added a comment.

- Add `*=` and `/=`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,65 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+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]
+}
+
+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]
+}
+
+void ok(double d) {
+  int i = 0;
+  i = 1;
+  i = static_cast(0.5);
+  i = static_cast(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(0.5);
+  i += static_cast(d);
+  i += std::ceil(0.5);
+  i += ::std::floor(0.5);
+  {
+using std::ceil;
+i += ceil(0.5f);
+  }
+  i += ceil(0.5f);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. 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.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -196,6 +196,11 @@
 
 - The 'google-runtime-member-string-references' check was removed.
 
+- New `cppcoreguidelines-narrowing-conversions
+  `_ check
+
+  Checks for narrowing conversions, e.g. ``int i = 0; i += 0.1;``.
+
 Improvements to include-fixer
 -
 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet marked 2 inline comments as done.
courbet added a comment.

Thanks.




Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32
+  Finder->addMatcher(
+  implicitCastExpr(hasImplicitDestinationType(isInteger()),
+   hasSourceExpression(IsFloatExpr),

JonasToth wrote:
> Do you plan to check for `double` -> `float` conversion, too?
> 
> Having a limited scope for now is ok, but a FIXME would be nice.
I've added a FIXME.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34
+   hasSourceExpression(IsFloatExpr),
+   unless(hasParent(castExpr(.bind("cast"),
+  this);

JonasToth wrote:
> 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(value);
> ```
> 
> or 
> 
> ```
> void some_function(int);
> 
> double d = 0.42;
> some_function(static_cast(d));
> ```
> would come to mind.
> 
> Nice to have, IMHO not necessary.
Luckily that does not seem to be implicit:

```
void f(double value) {
  int i = static_cast(value);
}
```

```
FunctionDecl 0x7fe5ffbd4150  line:1:6 f 'void (double)'
|-ParmVarDecl 0x7fe5ffbd4088  col:15 used value 'double'
`-CompoundStmt 0x7fe5ffbd4380 
  `-DeclStmt 0x7fe5ffbd4368 
`-VarDecl 0x7fe5ffbd4250  col:7 i 'int' cinit
  `-ImplicitCastExpr 0x7fe5ffbd4350  'int' 

`-CXXStaticCastExpr 0x7fe5ffbd4320  'float' 
static_cast 
  `-ImplicitCastExpr 0x7fe5ffbd4308  'float' 
`-ImplicitCastExpr 0x7fe5ffbd42f0  'double' 
  `-DeclRefExpr 0x7fe5ffbd42b0  'double' lvalue ParmVar 
0x7fe5ffbd4088 'value' 'double'
```






Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

JonasToth wrote:
> I would really like to add all other calc-and-assign operations. At least 
> "*=" and "/=".
Makes sense, I've added `*=` and `/=`. All others (`%=`, `&=`, `<<=` and 
variations thereof)  trigger a compilation error if the RHS is a float 
(rightfully).




Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:32
+  Finder->addMatcher(
+  implicitCastExpr(hasImplicitDestinationType(isInteger()),
+   hasSourceExpression(IsFloatExpr),

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

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



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:34
+   hasSourceExpression(IsFloatExpr),
+   unless(hasParent(castExpr(.bind("cast"),
+  this);

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(value);
```

or 

```
void some_function(int);

double d = 0.42;
some_function(static_cast(d));
```
would come to mind.

Nice to have, IMHO not necessary.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:41
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

In https://reviews.llvm.org/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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

> 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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 141610.
courbet edited the summary of this revision.
courbet added a comment.

- Add support for bad cast detection.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,60 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+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]
+}
+
+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]
+}
+
+void ok(double d) {
+  int i = 0;
+  i = 1;
+  i = static_cast(0.5);
+  i = static_cast(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(0.5);
+  i += static_cast(d);
+  i += std::ceil(0.5);
+  i += ::std::floor(0.5);
+  {
+using std::ceil;
+i += ceil(0.5f);
+  }
+  i += ceil(0.5f);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -76,6 +76,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. 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.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -196,6 +196,11 @@
 
 - The 'google-runtime-member-string-references' check was removed.
 
+- New `cppcoreguidelines-narrowing-conversions
+  `_ check
+
+  Checks for narrowing conversions, e.g. ``int i = 0; i += 0.1;``.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
@@ -0,0 +1,37 @@
+//===--- NarrowingConversionsCheck.h - 

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

Hi Jonas,

In https://reviews.llvm.org/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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Jonas Toth via Phabricator via cfe-commits]

JonasToth added a comment.

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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

ping


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added a comment.

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`.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh requested changes to this revision.
alexfh added a comment.
This revision now requires changes to proceed.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

aaron.ballman wrote:
> courbet wrote:
> > aaron.ballman wrote:
> > > 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).
> > 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.
> 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.
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.






Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

courbet wrote:
> aaron.ballman wrote:
> > 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).
> 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.
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

aaron.ballman wrote:
> 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).
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.


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.



Comment at: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp:26
+  binaryOperator(
+  anyOf(hasOperatorName("+="), hasOperatorName("-=")),
+  hasLHS(hasType(isInteger())),

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38455: [clang-tidy] new cppcoreguidelines-narrowing-conversions check.

[Clement Courbet via Phabricator via cfe-commits]

courbet updated this revision to Diff 138910.
courbet added a comment.
Herald added a subscriber: cfe-commits.

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


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D38455

Files:
  clang-tidy/cppcoreguidelines/CMakeLists.txt
  clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.cpp
  clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
  docs/ReleaseNotes.rst
  docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
  docs/clang-tidy/checks/list.rst
  test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp

Index: test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
===
--- /dev/null
+++ test/clang-tidy/cppcoreguidelines-narrowing-conversions.cpp
@@ -0,0 +1,38 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-narrowing-conversions %t
+
+float ceil(float);
+namespace std {
+double ceil(double);
+long double floor(long double);
+} // namespace std
+
+void not_ok(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]
+}
+
+void ok(double d) {
+  int i = 0;
+  i += 1;
+  i += static_cast(3.0);
+  i += static_cast(d);
+  i += std::ceil(3.0);
+  i += ::std::floor(3.0);
+  {
+using std::ceil;
+i += ceil(3.0f);
+  }
+  i += ceil(3.0f);
+}
Index: docs/clang-tidy/checks/list.rst
===
--- docs/clang-tidy/checks/list.rst
+++ docs/clang-tidy/checks/list.rst
@@ -74,6 +74,7 @@
cppcoreguidelines-avoid-goto
cppcoreguidelines-c-copy-assignment-signature (redirects to misc-unconventional-assign-operator) 
cppcoreguidelines-interfaces-global-init
+   cppcoreguidelines-narrowing-conversions
cppcoreguidelines-no-malloc
cppcoreguidelines-owning-memory
cppcoreguidelines-pro-bounds-array-to-pointer-decay
Index: docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
===
--- /dev/null
+++ docs/clang-tidy/checks/cppcoreguidelines-narrowing-conversions.rst
@@ -0,0 +1,13 @@
+.. 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.
Index: docs/ReleaseNotes.rst
===
--- docs/ReleaseNotes.rst
+++ docs/ReleaseNotes.rst
@@ -178,6 +178,11 @@
 - The 'misc-unused-raii' check was renamed to `bugprone-unused-raii
   `_
 
+- New `cppcoreguidelines-narrowing-conversions
+  `_ check
+
+  Checks for narrowing conversions, e.g. ``int i = 0; i += 0.1;``.
+
 Improvements to include-fixer
 -
 
Index: clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
===
--- /dev/null
+++ clang-tidy/cppcoreguidelines/NarrowingConversionsCheck.h
@@ -0,0 +1,37 @@
+//===--- 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