Skip to content

Commit

Permalink
Add an option to hicpp-signed-bitwise for positive integer literals.
Browse files Browse the repository at this point in the history
This gives developers a way to deviate from the coding standard to reduce the
chattiness of the check.
  • Loading branch information
VladimirPlyashkun authored and AaronBallman committed Oct 30, 2019
1 parent 0de262d commit 4de6b15
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 3 deletions.
17 changes: 16 additions & 1 deletion clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
Expand Up @@ -17,9 +17,24 @@ namespace clang {
namespace tidy {
namespace hicpp {

SignedBitwiseCheck::SignedBitwiseCheck(StringRef Name,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context),
IgnorePositiveIntegerLiterals(
Options.get("IgnorePositiveIntegerLiterals", false)) {}

void SignedBitwiseCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) {
Options.store(Opts, "IgnorePositiveIntegerLiterals",
IgnorePositiveIntegerLiterals);
}

void SignedBitwiseCheck::registerMatchers(MatchFinder *Finder) {
const auto SignedIntegerOperand =
expr(ignoringImpCasts(hasType(isSignedInteger()))).bind("signed-operand");
(IgnorePositiveIntegerLiterals
? expr(ignoringImpCasts(hasType(isSignedInteger())),
unless(integerLiteral()))
: expr(ignoringImpCasts(hasType(isSignedInteger()))))
.bind("signed-operand");

// The standard [bitmask.types] allows some integral types to be implemented
// as signed types. Exclude these types from diagnosing for bitwise or(|) and
Expand Down
7 changes: 5 additions & 2 deletions clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.h
Expand Up @@ -22,10 +22,13 @@ namespace hicpp {
/// http://clang.llvm.org/extra/clang-tidy/checks/hicpp-signed-bitwise.html
class SignedBitwiseCheck : public ClangTidyCheck {
public:
SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {}
SignedBitwiseCheck(StringRef Name, ClangTidyContext *Context);
void registerMatchers(ast_matchers::MatchFinder *Finder) override;
void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
void storeOptions(ClangTidyOptions::OptionMap &Options) override;

private:
bool IgnorePositiveIntegerLiterals;
};

} // namespace hicpp
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Expand Up @@ -103,6 +103,11 @@ Improvements to clang-tidy
Finds uses of deprecated Googletest APIs with names containing ``case`` and
replaces them with equivalent APIs with ``suite``.

- Improved :doc:`hicpp-signed-bitwise
<clang-tidy/checks/hicpp-signed-bitwise>` check.

The check now supports the ``IgnorePositiveIntegerLiterals`` option.

- New :doc:`linuxkernel-must-use-errs
<clang-tidy/checks/linuxkernel-must-use-errs>` check.

Expand Down
Expand Up @@ -7,3 +7,11 @@ Finds uses of bitwise operations on signed integer types, which may lead to
undefined or implementation defined behaviour.

The according rule is defined in the `High Integrity C++ Standard, Section 5.6.1 <http://www.codingstandard.com/section/5-6-shift-operators/>`_.

Options
-------

.. option:: IgnorePositiveIntegerLiterals

If this option is set to `true`, the check will not warn on bitwise operations with positive integer literals, e.g. `~0`, `2 << 1`, etc.
Default value is `false`.

5 comments on commit 4de6b15

@rillig
Copy link

@rillig rillig commented on 4de6b15 Nov 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pavelkryukov
Copy link

@pavelkryukov pavelkryukov commented on 4de6b15 Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman Could you please tell if it is possible to have that option in 9.0.1?

@AaronBallman
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not certain this would qualify for a dot release because it's adding a feature rather than fixing a bug (despite the feature being strongly related to a bug report). Is this change blocking something else?

@asl
Copy link
Collaborator

@asl asl commented on 4de6b15 Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. The basic rule is to fix the regressions only in dot releases. If this is fixed in the mainline, then just use the mainline.

@pavelkryukov
Copy link

@pavelkryukov pavelkryukov commented on 4de6b15 Dec 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. There is no blocking, just wanted to know when it is released.

Please sign in to comment.