Page MenuHomePhabricator

[clang-tidy] Exclude bitfield definitions from magic numbers check
ClosedPublic

Authored by 0x8000-0000 on Dec 4 2019, 7:45 PM.

Diff Detail

Event Timeline

0x8000-0000 created this revision.Dec 4 2019, 7:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 7:45 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
0x8000-0000 edited projects, added Restricted Project; removed Restricted Project.
0x8000-0000 edited subscribers, added: Restricted Project, Eugene.Zelenko, lebedev.ri; removed: cfe-commits.
Herald added a project: Restricted Project. · View Herald TranscriptDec 4 2019, 7:47 PM

Simplified the test cases by removing extraneous code.

MyDeveloperDay retitled this revision from Exclude bitfield definitions from magic numbers check to [clang-tidy] Exclude bitfield definitions from magic numbers check.Dec 5 2019, 2:23 AM

Document the new 'IgnoreBitFieldsWidths' option.

Please mention new option in Release Notes (changes section, in alphabetical order).

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
48

Unnecessary empty line.

49

Is clang namespace needed?

185

Unnecessary empty line.

0x8000-0000 marked 3 inline comments as done.Dec 5 2019, 3:21 PM
0x8000-0000 added inline comments.
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
49

Turns out it is needed.

/home/florin/tools/llvm-project/clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp:48:38: error: use of undeclared identifier 'FieldDecl'; did you mean 'fieldDecl'?
  const auto *AsFieldDecl = Node.get<FieldDecl>();
                                     ^~~~~~~~~
                                     fieldDecl

Please mention new option in Release Notes (changes section, in alphabetical order).

Thank you. Done.

Updated release notes. Removed empty lines.

aaron.ballman added inline comments.Dec 6 2019, 12:58 PM
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
49

I think the function should be made static and put into the clang namespace below (the same is true of the previous function in the anonymous namespace as well, but that doesn't need to be changed as part of this patch).

clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
48

I feel like there should be an assertion/unreachable in here -- there's no way for a bit-field width to be specified with a floating literal.

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

Is the default false because you want to preserve existing functionality? Or do you think bit-field widths as integer literals really are magic numbers?

Mordante added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
88

Can you also add a test for unsigned int: 0; to make sure it does not warn about this value? IMO it shouldn't warn since it is a special case with a well defined behaviour.

0x8000-0000 marked 3 inline comments as done.Dec 6 2019, 2:02 PM
0x8000-0000 added inline comments.
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.h
48

This follows the pattern with isSyntheticValue. It allows the checkBoundMatch template to be called with either. It is a hack (to keep most of the checks aligned between ints and float), that is getting close to the end of its useful life. But I'll break up that function on the next change.

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

The default is false to preserve the old behavior. I don't think the old behavior is that great so if somebody really wants it flipped I could easily go for it.

The bit fields as integer literals are magic, but I am not sure of the value of warning over them in code that accesses hardware directly and makes use of generated files. I would prefer people to use this check for the regular code, and not be scared away by many warnings that they can't fix anyway because it is vendor code.

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers.cpp
88

0 is a special value anyway, but I will add the test.

0x8000-0000 marked an inline comment as done.Dec 6 2019, 2:12 PM
0x8000-0000 added inline comments.
clang-tools-extra/clang-tidy/readability/MagicNumbersCheck.cpp
49

Done

Flipped the default to ignore bit fields. Moved static functions out of the anonymous namespace into the clang namespace.

0x8000-0000 marked an inline comment as done.Dec 6 2019, 3:07 PM
0x8000-0000 added inline comments.
clang-tools-extra/docs/clang-tidy/checks/readability-magic-numbers.rst
118

Flipped to ignore bit field widths by default.

aaron.ballman accepted this revision.Dec 7 2019, 6:49 AM

LGTM aside from a few small things to correct.

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

The documented default value is no longer correct.

clang-tools-extra/test/clang-tidy/checkers/readability-magic-numbers-bitfields.cpp
5

No need for this line any longer.

This revision is now accepted and ready to land.Dec 7 2019, 6:49 AM

LGTM aside from a few small things to correct.

Thank you Aaron. I don't have commit privileges so I'll need your help to integrate this.

Will you make the small fixes before commit, or would you like me to upload a new diff?

florin

LGTM aside from a few small things to correct.

Thank you Aaron. I don't have commit privileges so I'll need your help to integrate this.

Will you make the small fixes before commit, or would you like me to upload a new diff?

I'd appreciate if you could do the tweaks and upload a new patch. I'll try to apply sometime today.

Small documentation fixes.

aaron.ballman closed this revision.Dec 7 2019, 9:34 AM

Thanks for the patch, I've commit on your behalf in 6dcb1003f2022cba36e9f5a6d39648c3a3a213a0