[clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions

[paulaltin]

Summary

This change adds an option to disable warnings from the cppcoreguidelines-narrowing-conversions check on integer to floating-point conversions which may be narrowing.

An example of a case where this might be useful:

std::vector<double> v = {1, 2, 3, 4};
double mean = std::accumulate(v.cbegin(), v.cend(), 0.0) / v.size();

The conversion from std::size_t to double is technically narrowing on 64-bit systems, but v almost certainly does not have enough elements for this to be a problem.

This option would allow the cppcoreguidelines-narrowing-conversions check to be enabled on codebases which might otherwise turn it off because of cases like the above.

This change was modeled on https://reviews.llvm.org/D104018.

[Eugene.Zelenko]

Please mention improvement in Release Notes, in Changes in existing checks section. See example.

[paulaltin]

Fix pre-merge tests

[paulaltin]

Updated release notes.

[paulaltin]

Fix pre-merge tests.

[paulaltin]

Attempting to fix failed patch.

[paulaltin]

Attempting to fix failed patch.

[paulaltin]

In D112881#3100214, @Eugene.Zelenko wrote:

Please mention improvement in Release Notes, in Changes in existing checks section. See example.

Thanks @Eugene.Zelenko, and apologies for the n00b mistakes (this is my first Phabricator submission).

Would you be able to help me understand why the pre-merge checks are failing?

[paulaltin]

Ping.

I'm a bit stuck with this submission, any help or pointers on how to proceed (i.e. how to fix the pre-merge tests) would be much appreciated.

[salman-javed-nz]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp

13

Build is failing because you don't have a CHECK-MESSAGES-DISABLED line anywhere in the file.
You could change // DISABLED: to // CHECK-MESSAGES-DISABLED-NOT: and check for the absence of the check warning.

[salman-javed-nz]

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp

13

[gchatelet]

LGTM once tests are passing. Maybe wait a bit for a comment from @aaron.ballman .

[paulaltin]

Add a CHECK-MESSAGES-DISABLED test in an attempt to fix pre-merge checks.

[paulaltin]

Fixing bad diff formatting.

[paulaltin]

Fixing bad diff formatting.

[paulaltin]

Fixing syntax error.

[paulaltin]

Fixing column number.

[paulaltin]

Thanks for your help @salman-javed-nz.

Build is failing because you don't have a CHECK-MESSAGES-DISABLED line anywhere in the file.
You could change DISABLED: to CHECK-MESSAGES-DISABLED-NOT: and check for the absence of the check warning.

Instead of doing this, I added another test function which checks that floating-point to integer warnings still work when this option is enabled. This seems to be more in line with what other option tests do (e.g. cppcoreguidelines-narrowing-conversions-equivalentbitwidth-option.cpp).

[paulaltin]

In D112881#3155200, @gchatelet wrote:

LGTM once tests are passing. Maybe wait a bit for a comment from @aaron.ballman .

Thanks @gchatelet. Happy to wait for @aaron.ballman to comment.

[aaron.ballman]

LGTM, with a few small nits. It'd be nice to update the patch summary with more information about why the option is needed.

clang-tools-extra/docs/ReleaseNotes.rst

122 ↗

(On Diff #390136)

Can you add backticks around WarnOnIntegerToFloatingPointNarrowingConversion so it gets highlighted properly, and reflow to the usual 80-col limit (it's fine to ignore it when links get too long, but not fine for a whole paragraph).

[paulaltin]

Fixing Release Notes formatting.

[paulaltin]

LGTM, with a few small nits. It'd be nice to update the patch summary with more information about why the option is needed.

Thanks @aaron.ballman. I've made the changes to the Release Notes and added more info to the summary.

In general, would you be happy to consider other changes like this in clang-tidy? Our company uses the tool as part of our CI (and I personally find it extremely useful), but there are some checks that we have to disable entirely because they do multiple things, some of which we don't want to (or can't easily) fix. If these had options to disable part of the check then we would still be able to use the remaining parts, which would be great.

If adding more options to existing checks in clang-tidy is a direction that the developers are happy to support, then I'd be keen to submit more changes like this.

[aaron.ballman]

In D112881#3160590, @paulaltin wrote:

LGTM, with a few small nits. It'd be nice to update the patch summary with more information about why the option is needed.

Thanks @aaron.ballman. I've made the changes to the Release Notes and added more info to the summary.

Thanks! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

In general, would you be happy to consider other changes like this in clang-tidy? Our company uses the tool as part of our CI (and I personally find it extremely useful), but there are some checks that we have to disable entirely because they do multiple things, some of which we don't want to (or can't easily) fix. If these had options to disable part of the check then we would still be able to use the remaining parts, which would be great.

If adding more options to existing checks in clang-tidy is a direction that the developers are happy to support, then I'd be keen to submit more changes like this.

We're definitely happy to consider these kinds of changes, thank you! We usually prefer finding ways to silence the diagnostic in code (like casting to void to silence an unused variable warning, etc) over configuration options, but that's not always plausible to do and so extra configuration options are a good fallback solution.

[paulaltin]

Thanks! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

That would be great, thanks! You can use "Paul Altin <[e-mail]>" for the attribution.

We're definitely happy to consider these kinds of changes, thank you! We usually prefer finding ways to silence the diagnostic in code (like casting to void to silence an unused variable warning, etc) over configuration options, but that's not always plausible to do and so extra configuration options are a good fallback solution.

Ok, fantastic. I'll go back through the checks that we've disabled and see what I can suggest.

Thanks @aaron.ballman!

[aaron.ballman]

In D112881#3181082, @paulaltin wrote:

Thanks! Do you need someone to commit on your behalf? If so, what name and email address would you like used for patch attribution?

That would be great, thanks! You can use "Paul Altin <[e-mail]>" for the attribution.

Thanks! I landed this as 9198d04c06b561cd78d9407cedd50f7b995ee6d8 this morning (sorry for the slight delay in getting this landed).

[paulaltin]

Thanks! I landed this as 9198d04c06b561cd78d9407cedd50f7b995ee6d8 this morning (sorry for the slight delay in getting this landed).

No worries, thanks @aaron.ballman!

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

                                           const BuiltinType &ToType) const;
 
   const bool WarnOnIntegerNarrowingConversion;
+  const bool WarnOnIntegerToFloatingPointNarrowingConversion;
   const bool WarnOnFloatingPointNarrowingConversion;
   const bool WarnWithinTemplateInstantiation;
   const bool WarnOnEquivalentBitWidth;

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

     : ClangTidyCheck(Name, Context),
       WarnOnIntegerNarrowingConversion(
           Options.get("WarnOnIntegerNarrowingConversion", true)),
+      WarnOnIntegerToFloatingPointNarrowingConversion(
+          Options.get("WarnOnIntegerToFloatingPointNarrowingConversion", true)),
       WarnOnFloatingPointNarrowingConversion(
           Options.get("WarnOnFloatingPointNarrowingConversion", true)),
       WarnWithinTemplateInstantiation(

     ClangTidyOptions::OptionMap &Opts) {
   Options.store(Opts, "WarnOnIntegerNarrowingConversion",
                 WarnOnIntegerNarrowingConversion);
+  Options.store(Opts, "WarnOnIntegerToFloatingPointNarrowingConversion",
+                WarnOnIntegerToFloatingPointNarrowingConversion);
   Options.store(Opts, "WarnOnFloatingPointNarrowingConversion",
                 WarnOnFloatingPointNarrowingConversion);
   Options.store(Opts, "WarnWithinTemplateInstantiation",

 void NarrowingConversionsCheck::handleIntegralToFloating(
     const ASTContext &Context, SourceLocation SourceLoc, const Expr &Lhs,
     const Expr &Rhs) {
-  const BuiltinType *ToType = getBuiltinType(Lhs);
-  llvm::APSInt IntegerConstant;
-  if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) {
-    if (!isWideEnoughToHold(Context, IntegerConstant, *ToType))
-      diagNarrowIntegerConstant(SourceLoc, Lhs, Rhs, IntegerConstant);
-    return;
-  }
+  if (WarnOnIntegerToFloatingPointNarrowingConversion) {
+    const BuiltinType *ToType = getBuiltinType(Lhs);
+    llvm::APSInt IntegerConstant;
+    if (getIntegerConstantExprValue(Context, Rhs, IntegerConstant)) {
+      if (!isWideEnoughToHold(Context, IntegerConstant, *ToType))
+        diagNarrowIntegerConstant(SourceLoc, Lhs, Rhs, IntegerConstant);
+      return;
+    }
 
     const BuiltinType *FromType = getBuiltinType(Rhs);
     if (isWarningInhibitedByEquivalentSize(Context, *FromType, *ToType))
       return;
     if (!isWideEnoughToHold(Context, *FromType, *ToType))
       diagNarrowType(SourceLoc, Lhs, Rhs);
+  }
 }
 
 void NarrowingConversionsCheck::handleFloatingToIntegral(

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

 We enforce only part of the guideline, more specifically, we flag narrowing conversions from:
  - an integer to a narrower integer (e.g. ``char`` to ``unsigned char``)
    if WarnOnIntegerNarrowingConversion Option is set,
- - an integer to a narrower floating-point (e.g. ``uint64_t`` to ``float``),
+ - an integer to a narrower floating-point (e.g. ``uint64_t`` to ``float``)
+   if WarnOnIntegerToFloatingPointNarrowingConversion Option is set,
  - a floating-point to an integer (e.g. ``double`` to ``int``),
  - a floating-point to a narrower floating-point (e.g. ``double`` to ``float``)
    if WarnOnFloatingPointNarrowingConversion Option is set.

     When `true`, the check will warn on narrowing integer conversion
     (e.g. ``int`` to ``size_t``). `true` by default.
 
+.. option:: WarnOnIntegerToFloatingPointNarrowingConversion
+
+    When `true`, the check will warn on narrowing integer to floating-point
+    conversion (e.g. ``size_t`` to ``double``). `true` by default.
+
 .. option:: WarnOnFloatingPointNarrowingConversion
 
     When `true`, the check will warn on narrowing floating point conversion

clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines-narrowing-conversions-narrowingintegertofloatingpoint-option.cpp

+// RUN: %check_clang_tidy -check-suffix=DEFAULT %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [{key: cppcoreguidelines-narrowing-conversions.WarnOnIntegerToFloatingPointNarrowingConversion, value: true}]}'
+
+// RUN: %check_clang_tidy -check-suffix=DISABLED %s \
+// RUN: cppcoreguidelines-narrowing-conversions %t -- \
+// RUN: -config='{CheckOptions: [{key: cppcoreguidelines-narrowing-conversions.WarnOnIntegerToFloatingPointNarrowingConversion, value: false}]}'
+
+void foo(size_t value) {
+  double a = value;
+  // CHECK-MESSAGES-DEFAULT: :[[@LINE-1]]:11: warning: narrowing conversion from 'size_t' (aka 'unsigned long') to 'double' [cppcoreguidelines-narrowing-conversions]
+  // DISABLED: No warning for integer to floating-point narrowing conversions when WarnOnIntegerToFloatingPointNarrowingConversion = false.
+}