This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Change readability-magic-numbers to allow numbers in type aliases.
ClosedPublic

Authored by florianhumblot on Mar 10 2023, 2:31 AM.

Details

Summary

This commit introduces an option to the readability-magic-values checker.
The option defaults to false so that the behavior of the checker doesn't change unless specifically enabled.
These commits are supposed to fix https://github.com/llvm/llvm-project/issues/61259

Diff Detail

Event Timeline

florianhumblot created this revision.Mar 10 2023, 2:31 AM
florianhumblot requested review of this revision.Mar 10 2023, 2:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2023, 2:31 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
PiotrZSL added inline comments.Mar 10 2023, 2:36 AM
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
40

This should be configurable, someone else may don't want magic numbers in template arguments.
So we shouldn't change default behaviour.

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

consider: "check, now allows..."

PiotrZSL requested changes to this revision.Mar 10 2023, 2:55 AM

This is for issue: https://github.com/llvm/llvm-project/issues/61259
Reference it in commit message.

Add config: "flag should be added i.e. IgnoreTypeAliases which will default to false."
Except that looks fine.

This revision now requires changes to proceed.Mar 10 2023, 2:55 AM
florianhumblot edited the summary of this revision. (Show Details)

Made it so that the proposed change doesn't change the default behavior of the check.
Introduce an option to allow enabling the exclusion.

"Context not available." - use arcanist (just merge all local commits into one), or generate diff with full context (diff -U 9999999).

clang-tools-extra/docs/clang-tidy/checks/readability/magic-numbers.rst
58

example put under "Example with magic values refactored:" should be by default as (+-):

using containerType = CustomType<int, NUMBER_OF_ELEMENTS>;

as this is required by default.

florianhumblot marked 2 inline comments as done.

Generate diff with full context and updated the example to use the default settings of the readabiliy-magic-numbers checker

florianhumblot marked an inline comment as done.Mar 10 2023, 4:44 AM
This revision is now accepted and ready to land.Mar 10 2023, 5:07 AM

LGTM

Thanks for the reviews @PiotrZSL !
Is one review enough or does the second reviewer also need to approve these changes?

If this one is enough, could I ask you (or someone else) to commit the change since I don't have commit access? :)

What should I put as commit author ?
I'm asking because this is diff, and commit author looks to be attached to it.

What should I put as commit author ?
I'm asking because this is diff, and commit author looks to be attached to it.

"Florian Humblot <me@florianhumblot.com>" should be good :)