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

[0x8000-0000]

Simplified the test cases by removing extraneous code.

[0x8000-0000]

Document the new 'IgnoreBitFieldsWidths' option.

[Eugene.Zelenko]

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]

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

[0x8000-0000]

In D71043#1770695, @Eugene.Zelenko wrote:

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

Thank you. Done.

[0x8000-0000]

Updated release notes. Removed empty lines.

[aaron.ballman]

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]

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]

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]

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

49

Done

[0x8000-0000]

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

[0x8000-0000]

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

118

Flipped to ignore bit field widths by default.

[aaron.ballman]

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.

[0x8000-0000]

In D71043#1773977, @aaron.ballman wrote:

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

[aaron.ballman]

In D71043#1773982, @0x8000-0000 wrote:

In D71043#1773977, @aaron.ballman wrote:

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.

[0x8000-0000]

Small documentation fixes.

[aaron.ballman]

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

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

                         const FloatingLiteral *) const {
     return false;
   }
-
   bool isSyntheticValue(const clang::SourceManager *SourceManager,
                         const IntegerLiteral *Literal) const;
 
+  bool isBitFieldWidth(const clang::ast_matchers::MatchFinder::MatchResult &,
+                       const FloatingLiteral &) const {
+     return false;
+  }
+
+  bool isBitFieldWidth(const clang::ast_matchers::MatchFinder::MatchResult &Result,
+                       const IntegerLiteral &Literal) const;
+
   template <typename L>
   void checkBoundMatch(const ast_matchers::MatchFinder::MatchResult &Result,
                        const char *BoundName) {

     if (isSyntheticValue(Result.SourceManager, MatchedLiteral))
       return;
 
+    if (isBitFieldWidth(Result, *MatchedLiteral))
+      return;
+
     const StringRef LiteralSourceText = Lexer::getSourceText(
         CharSourceRange::getTokenRange(MatchedLiteral->getSourceRange()),
         *Result.SourceManager, getLangOpts());

   }
 
   const bool IgnoreAllFloatingPointValues;
+  const bool IgnoreBitFieldsWidths;
   const bool IgnorePowersOf2IntegerValues;
 
   constexpr static unsigned SensibleNumberOfMagicValueExceptions = 16;

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

 using namespace clang::ast_matchers;
 using namespace clang::ast_type_traits;
 
-namespace {
+namespace clang {
 
-bool isUsedToInitializeAConstant(const MatchFinder::MatchResult &Result,
+static bool isUsedToInitializeAConstant(const MatchFinder::MatchResult &Result,
                                         const DynTypedNode &Node) {
 
-  const auto *AsDecl = Node.get<clang::DeclaratorDecl>();
+  const auto *AsDecl = Node.get<DeclaratorDecl>();
   if (AsDecl) {
     if (AsDecl->getType().isConstQualified())
       return true;

     return AsDecl->isImplicit();
   }
 
-  if (Node.get<clang::EnumConstantDecl>() != nullptr)
+  if (Node.get<EnumConstantDecl>() != nullptr)
     return true;
 
   return llvm::any_of(Result.Context->getParents(Node),

                       });
 }
 
-} // namespace
+static bool isUsedToDefineABitField(const MatchFinder::MatchResult &Result,
+                                    const DynTypedNode &Node) {
+  const auto *AsFieldDecl = Node.get<FieldDecl>();
+  if (AsFieldDecl && AsFieldDecl->isBitField())
+    return true;
+
+  return llvm::any_of(Result.Context->getParents(Node),
+                      [&Result](const DynTypedNode &Parent) {
+                        return isUsedToDefineABitField(Result, Parent);
+                      });
+}
 
-namespace clang {
 namespace tidy {
 namespace readability {
 

     : ClangTidyCheck(Name, Context),
       IgnoreAllFloatingPointValues(
           Options.get("IgnoreAllFloatingPointValues", false)),
+      IgnoreBitFieldsWidths(Options.get("IgnoreBitFieldsWidths", true)),
       IgnorePowersOf2IntegerValues(
           Options.get("IgnorePowersOf2IntegerValues", false)) {
   // Process the set of ignored integer values.

   return BufferIdentifier.empty();
 }
 
+bool MagicNumbersCheck::isBitFieldWidth(
+    const clang::ast_matchers::MatchFinder::MatchResult &Result,
+    const IntegerLiteral &Literal) const {
+  return IgnoreBitFieldsWidths &&
+         llvm::any_of(Result.Context->getParents(Literal),
+                      [&Result](const DynTypedNode &Parent) {
+                        return isUsedToDefineABitField(Result, Parent);
+                      });
+}
+
 } // namespace readability
 } // namespace tidy
 } // namespace clang

clang-tools-extra/docs/ReleaseNotes.rst

   Finds classes, structs, and unions that contain redundant member
   access specifiers.
 
+- Improved :doc:`readability-magic-numbers
+  <clang-tidy/checks/readability-magic-numbers>` check.
+
+  The check now supports the ``IgnoreBitFieldsWidths`` option to suppress
+  the warning for numbers used to specify bit field widths.
+
 - New :doc:`readability-make-member-function-const
   <clang-tidy/checks/readability-make-member-function-const>` check.
 

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

    Boolean value indicating whether to accept all floating point values without
    warning. Default value is `false`.
 
+.. option:: IgnoreBitFieldsWidths
+
+   Boolean value indicating whether to accept magic numbers as bit field widths
+   without warning. This is useful for example for register definitions which
+   are generated from hardware specifications. Default value is `true`.

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

+// RUN: %check_clang_tidy %s readability-magic-numbers %t \
+// RUN: -config='{CheckOptions: \
+// RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "1;2;10;100;"}]}' \
+// RUN: --
+
+struct HardwareGateway {
+   /*
+    * The configuration suppresses the warnings for the bitfields...
+    */
+   unsigned int Some: 5;
+   unsigned int Bits: 7;
+   unsigned int: 7;
+   unsigned int: 0;
+   unsigned int Rest: 13;
+
+   /*
+    * ... but other fields trigger the warning.
+    */
+   unsigned int Another[3];
+   // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+};
+

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

 // RUN: -config='{CheckOptions: \
 // RUN:  [{key: readability-magic-numbers.IgnoredIntegerValues, value: "0;1;2;10;100;"}, \
 // RUN:   {key: readability-magic-numbers.IgnoredFloatingPointValues, value: "3.14;2.71828;9.81;10000.0;101.0;0x1.2p3"}, \
+// RUN:   {key: readability-magic-numbers.IgnoreBitFieldsWidths, value: 0}, \
 // RUN:   {key: readability-magic-numbers.IgnorePowersOf2IntegerValues, value: 1}]}' \
 // RUN: --
 

   // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
 }
 
+struct HardwareGateway {
+   unsigned int Some: 5;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 5 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int Bits: 7;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 7 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int: 6;
+   // CHECK-MESSAGES: :[[@LINE-1]]:18: warning: 6 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   unsigned int Flag: 1; // no warning since this is suppressed by IgnoredIntegerValues rule
+   unsigned int: 0;      // no warning since this is suppressed by IgnoredIntegerValues rule
+   unsigned int Rest: 13;
+   // CHECK-MESSAGES: :[[@LINE-1]]:23: warning: 13 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+   //
+   unsigned int Another[3];
+   // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: 3 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
+};
+
+
 /*
  * Clean code
  */