This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by paulaltin on Oct 30 2021, 8:21 PM.

Details

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.

Diff Detail

Event Timeline

paulaltin created this revision.Oct 30 2021, 8:21 PM
paulaltin requested review of this revision.Oct 30 2021, 8:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2021, 8:21 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
paulaltin retitled this revision from Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions to [clang-tidy] Allow disabling integer to floating-point narrowing conversions for cppcoreguidelines-narrowing-conversions.
paulaltin updated this revision to Diff 383631.Oct 30 2021, 8:25 PM
paulaltin edited the summary of this revision. (Show Details)
Eugene.Zelenko set the repository for this revision to rG LLVM Github Monorepo.

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

paulaltin updated this revision to Diff 383893.Nov 1 2021, 2:44 PM

Fix pre-merge tests

paulaltin updated this revision to Diff 383898.Nov 1 2021, 2:52 PM

Updated release notes.

paulaltin updated this revision to Diff 383901.Nov 1 2021, 2:53 PM

Fix pre-merge tests.

paulaltin updated this revision to Diff 383904.Nov 1 2021, 3:00 PM

Attempting to fix failed patch.

paulaltin updated this revision to Diff 383905.Nov 1 2021, 3:03 PM

Attempting to fix failed patch.

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?

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 added inline comments.
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.

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

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

paulaltin updated this revision to Diff 390126.Nov 26 2021, 5:56 PM

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

paulaltin updated this revision to Diff 390127.Nov 26 2021, 6:03 PM

Fixing bad diff formatting.

paulaltin updated this revision to Diff 390129.Nov 26 2021, 6:31 PM

Fixing bad diff formatting.

paulaltin updated this revision to Diff 390131.Nov 26 2021, 6:49 PM

Fixing syntax error.

paulaltin updated this revision to Diff 390136.Nov 26 2021, 7:21 PM

Fixing column number.

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

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 accepted this revision.Nov 29 2021, 8:36 AM

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

This revision is now accepted and ready to land.Nov 29 2021, 8:36 AM
paulaltin updated this revision to Diff 390573.EditedNov 29 2021, 10:53 PM
paulaltin edited the summary of this revision. (Show Details)

Fixing Release Notes formatting.

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.

paulaltin edited the summary of this revision. (Show Details)Nov 30 2021, 12:44 AM

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.

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 <paul@liquidinstruments.com>" 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 closed this revision.Dec 16 2021, 5:26 AM

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 <paul@liquidinstruments.com>" for the attribution.

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

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

No worries, thanks @aaron.ballman!