This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] Fix bug 34845, offending standard bitmask types
ClosedPublic

Authored by JonasToth on Oct 19 2017, 9:42 AM.

Details

Summary

The C++ standard allows implementations to choose the underlying type for
bitmask types (e.g. std::ios_base::openmode). MSVC implemented some of them
as signed integers resulting in warnings for usual code like
auto dd = std::ios_base::badbit | std::ios_base::failbit;

These false positives were reported in https://bugs.llvm.org/show_bug.cgi?id=34845

The fix allows bitwise |,&,^ for known standard bitmask types under the condition
that both operands are such bitmask types.
Shifting and bitwise complement are still forbidden.

Event Timeline

JonasToth created this revision.Oct 19 2017, 9:42 AM
JonasToth updated this revision to Diff 119595.Oct 19 2017, 9:44 AM
  • fix run line for unit test, unwanted change
JonasToth updated this revision to Diff 119596.Oct 19 2017, 9:47 AM
  • remove debug output
aaron.ballman added inline comments.Oct 20 2017, 7:49 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
80

No need to re-initialize the value here. You can drop it here or remove the initialization above.

84

Drop the newline.

88

Drop the newline.

test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
7

because the appear -> because they appear

I think a better way to handle this might be to put the enumerations into a local header file that uses #pragma clang system_header, and then #include that file to get the declarations in the test.

JonasToth marked 4 inline comments as done.Oct 20 2017, 10:26 AM
JonasToth added inline comments.
test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
7

Typo is fixed. I copy&pasted the definitions into a header file like you suggested, but then 'check_clang_tidy' is not happy. It does not seem to find the included header.

I believe, this is due to the path from where it is executed? Having it run manually works though.
Maybe I could just leave them here?

aaron.ballman added inline comments.Oct 20 2017, 10:56 AM
test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
7

I would strongly prefer not to, as we also want to test that these warnings do not appear in system headers. I am not certain why check_clang_tidy is unable to find the header file, especially if the header is next to the source file. That would be worth tracking down as a deficiency with check_clang_tidy.

JonasToth marked an inline comment as done.
  • address review comments
  • fix run line, check-clang-tidy will not be invoked
JonasToth added inline comments.Oct 20 2017, 11:59 AM
test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
7

What works is using 'clang-tidy' directly in the RUN line.

I tried a little bit to figure out why clang-tidy-check does not do it correctly, but i currently have not enough time.
Maybe i can investigate it later this weekend or next week.

JonasToth marked 3 inline comments as done.Oct 20 2017, 11:59 AM
aaron.ballman added inline comments.Oct 27 2017, 6:37 AM
test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
4

Why does this include need to go up a directory level? The header file resides next to the source file, so I would expect this to not find the include.

test/clang-tidy/hicpp-signed-bitwise-standard-types.h
82 ↗(On Diff #119678)

Extra newline can be removed.

JonasToth added inline comments.Oct 27 2017, 7:23 AM
test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
4

Inspecting the testoutput, check-clang-tidy does create its own directory from where its run. I tried both version (with '../' and without) and both are functional. I can remove if you want.

JonasToth updated this revision to Diff 120600.Oct 27 2017, 7:27 AM
  • adjust run line and include
  • remove whitespace
JonasToth marked an inline comment as done.Oct 27 2017, 7:31 AM
JonasToth updated this revision to Diff 120602.Oct 27 2017, 7:33 AM
  • remove conflict markers
aaron.ballman accepted this revision.Oct 27 2017, 7:35 AM

LGTM!

test/clang-tidy/hicpp-signed-bitwise-standard-types.cpp
4

It's less confusing with it removed.

This revision is now accepted and ready to land.Oct 27 2017, 7:35 AM
JonasToth marked 3 inline comments as done.Oct 27 2017, 7:41 AM
This revision was automatically updated to reflect the committed changes.