Page MenuHomePhabricator

[clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands
ClosedPublic

Authored by vladimir.plyashkun on Oct 9 2019, 4:32 AM.

Details

Summary

Hi! Clang-Tidy has been integrated in CLion some time ago, but a lot of users started to complain on hicpp-signed-bitwise inspection as it produces a lot of noise, especially for positive integer literals.

There are already some issues (with discussions) in LLVM tracker with same problem:
https://bugs.llvm.org/show_bug.cgi?id=36961
https://bugs.llvm.org/show_bug.cgi?id=43606

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

#include <fcntl.h>

int main() {
    open("file", O_RDONLY | O_NOCTTY); // <-- warning here
}

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 4:32 AM
aaron.ballman added a subscriber: aaron.ballman.

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

I think that this check should behave how the coding standard dictates it behaves. By my reading of HIC++, the check is behaving by design. I think there are a few ways forward though (not mutually exclusive):

0) See if the HIC++ folks are interested in clarifying their standard for integer literals and if they are, modify the check

  1. Add an off-by-default option that suppresses the diagnostic with positive integer literals so that users can explicitly decide to deviate locally
  2. Document that this happens and is expected behavior; users can disable the check if they don't wish to conform to that standard.
lebedev.ri retitled this revision from Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands to [clang-tidy] hicpp-signed-bitwise: Do not show "use of a signed integer operand with a binary bitwise operator" for positive integer operands.Oct 9 2019, 10:30 AM

In my opinion this check should be disabled in case of integer literals, since there are a lot of existing code (even in system libraries) where user can do nothing, e.g.:

I think that this check should behave how the coding standard dictates it behaves. By my reading of HIC++, the check is behaving by design. I think there are a few ways forward though (not mutually exclusive):

0) See if the HIC++ folks are interested in clarifying their standard for integer literals and if they are, modify the check

  1. Add an off-by-default option that suppresses the diagnostic with positive integer literals so that users can explicitly decide to deviate locally
  2. Document that this happens and is expected behavior; users can disable the check if they don't wish to conform to that standard.

Hey :)
I would personally prefer option 1. My biggest wish is to have time to come back to development in a few weeks (but not guaranteed :/) then I would iterate the check a bit to make it more userfriendly as well!

Best, Jonas

Provide additional option to preserve current inspection behaviour

@aaron.ballman
@JonasToth
Thanks, i agree too! I've prepared new revision with additional option to preserve old inspection behaviour.

lebedev.ri added inline comments.
clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
23–24

i'm not sure this should look for a global option with such name?

The new switch needs documentation as well, and maybe even a note in the release notes (as it is publicly discussed as issue?).
Otherwise I am fine with the changes!

clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
20

Could you please add URes << 1 as well? I believe that was problematic in the stack-overflow-question, wasn't it?

vladimir.plyashkun marked an inline comment as done.Oct 11 2019, 4:37 AM
vladimir.plyashkun added inline comments.
clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
23–24

I think that this method is common and used in so many inspections.
For example this check also have option called IgnoreMacros which is retrieved in same way (by calling getLocalOrGlobal method)

lebedev.ri added inline comments.Oct 11 2019, 4:46 AM
clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
23–24

I'm very specifically discriminating against "IgnorePositiveIntegerLiterals" here.
I know getLocalOrGlobal() is widely used, because in those places the same flag
is used in multiple modules with same meaning.
Is that the case here?

vladimir.plyashkun marked 2 inline comments as done.Oct 11 2019, 8:08 AM
vladimir.plyashkun added inline comments.
clang-tools-extra/clang-tidy/hicpp/SignedBitwiseCheck.cpp
23–24

Ok, i agree. Fixed.

clang-tools-extra/test/clang-tidy/hicpp-signed-bitwise-integer-literals.cpp
20

Yes, sure.

The new switch needs documentation as well, and maybe even a note in the release notes (as it is publicly discussed as issue?).

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

The files reside in clang-tools-extra/docs/ReleaseNotes.rst and clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst.

You can check other files for reference, e.g. readability-magic-numbers.rst.
In the Release Notes you can add one entry at the end of the clang-tidy section.

Do you know who is responsible for it? Because i haven't worked with documentation before and don't know what i need to do to update it.

The files reside in clang-tools-extra/docs/ReleaseNotes.rst and clang-tools-extra/docs/clang-tidy/checks/hicpp-signed-bitwise.rst.

You can check other files for reference, e.g. readability-magic-numbers.rst.
In the Release Notes you can add one entry at the end of the clang-tidy section.

Thanks! I've fixed documentation.

JonasToth accepted this revision.Oct 15 2019, 1:43 PM

LGTM.
But i was inactive for a long time, if @aaron.ballman accepts as well you can commit instantly. Otherwise please let the other people that commented some time to react. :)

This revision is now accepted and ready to land.Oct 15 2019, 1:43 PM
aaron.ballman accepted this revision.Oct 16 2019, 7:26 AM

LGTM aside from a small nit.

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

IgnorePositiveIntegerLiterals should use two backticks instead of one.

vladimir.plyashkun marked an inline comment as done.

LGTM aside from a small nit.

Thanks, fixed it!

Thank you for the patch! I've committed on your behalf in 4de6b1586807285e20a5db6596519c2336a64568.