This is an archive of the discontinued LLVM Phabricator instance.

[clang-tidy] add missing assignment operations in hicpp-signed-bitwise
ClosedPublic

Authored by JonasToth on Apr 8 2018, 7:37 AM.

Details

Summary

This patch resolves the bug https://bugs.llvm.org/show_bug.cgi?id=36963.

  • implement missing assignment operators for hicpp-signed-bitwise
  • add tests
  • mention fix in release notes

Event Timeline

JonasToth created this revision.Apr 8 2018, 7:37 AM
JonasToth updated this revision to Diff 141547.Apr 8 2018, 7:54 AM
  • fix std bitmask type shifting issue
aaron.ballman accepted this revision.Apr 10 2018, 8:20 AM

LG aside from a diagnostic wording nit.

clang-tidy/hicpp/SignedBitwiseCheck.cpp
92

How about: "shifting a value of bitmask type"

This revision is now accepted and ready to land.Apr 10 2018, 8:20 AM
JonasToth added inline comments.Apr 10 2018, 9:35 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
92

Not sure about that.

The general bitmasks are covered by the second diag.

This one should only trigger if a shift with the standardized bitmask types occurs. This exception is necessary because those are allowed for the other bitwise operations(&, |, ^), but i decided that shifting them makes no sense.

aaron.ballman added inline comments.Apr 10 2018, 9:45 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
92

I think "standardized bitmask type" is not very clear because it's hard to know what is and isn't a standardized bitmask type (it's not a term of art I'm used to hearing, anyway). I agree that shifting by them makes no sense, but I also have a hard time imagining anyone is using these as shift values in the first place, so perhaps the diagnostic can be removed entirely unless we can find some code in the wild that does something like this?

JonasToth added inline comments.Apr 10 2018, 10:01 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
92

I totally agree that its bad.

Removing this exception is ok with me.

aaron.ballman added inline comments.Apr 10 2018, 10:22 AM
clang-tidy/hicpp/SignedBitwiseCheck.cpp
92

Okay, we'll go that route -- we can always bring it back if it turns out to have value later. Thanks!

JonasToth updated this revision to Diff 141970.Apr 11 2018, 2:51 AM
  • remove shifting exception for standarized bitmask types
JonasToth marked 5 inline comments as done.Apr 11 2018, 2:52 AM