[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL297761: Warn on enum assignment to bitfields that can't fit all values (authored by rnk). Changed prior to commit: https://reviews.llvm.org/D30923?vs=91749=91752#toc Repository: rL LLVM

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
rnk marked an inline comment as done. rnk added a comment. Cool, thanks for the reviews, this is definitely in better shape now. This is off by default, so I'm going to commit and experiment with it on a few codebases. I'd still like to hear if either of the Richards have diagnostic wording

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Hal Finkel via Phabricator via cfe-commits
hfinkel added a comment. In https://reviews.llvm.org/D30923#700780, @rnk wrote: > In https://reviews.llvm.org/D30923#700708, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700696, @rnk wrote: > > > > > Do you think it's worth indicating that the error can be suppressed with > > > an

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment. In https://reviews.llvm.org/D30923#700708, @hfinkel wrote: > In https://reviews.llvm.org/D30923#700696, @rnk wrote: > > > Do you think it's worth indicating that the error can be suppressed with an > > explicit cast, or is that wasted space? > > > What might this look like?

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
rnk updated this revision to Diff 91749. rnk added a comment. - Test that explicit casts suppress the warning https://reviews.llvm.org/D30923 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Hal Finkel via Phabricator via cfe-commits
hfinkel added a comment. In https://reviews.llvm.org/D30923#700696, @rnk wrote: > In https://reviews.llvm.org/D30923#700626, @hfinkel wrote: > > > In https://reviews.llvm.org/D30923#700620, @thakis wrote: > > > > > Maybe it should have some "to suppress the warning, do X" fixit? > > > > > > I

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
rnk updated this revision to Diff 91741. rnk added a comment. - Beef up test as Nico suggests - Add two notes, one to change the bitfield signedness, one to indicate the required bitwidth https://reviews.llvm.org/D30923 Files: include/clang/Basic/DiagnosticGroups.td

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Reid Kleckner via Phabricator via cfe-commits
rnk added a comment. In https://reviews.llvm.org/D30923#700626, @hfinkel wrote: > In https://reviews.llvm.org/D30923#700620, @thakis wrote: > > > Maybe it should have some "to suppress the warning, do X" fixit? > > > I think we should at least include how many bits the field should have to fix

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Hal Finkel via Phabricator via cfe-commits
hfinkel added a comment. In https://reviews.llvm.org/D30923#700620, @thakis wrote: > Maybe it should have some "to suppress the warning, do X" fixit? I think we should at least include how many bits the field should have to fix the problem (pointing to the relevant field definition certainly

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment. Maybe it should have some "to suppress the warning, do X" fixit? https://reviews.llvm.org/D30923 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-14 Thread Nico Weber via Phabricator via cfe-commits
thakis accepted this revision. thakis added a comment. This revision is now accepted and ready to land. Looks like a cool warning. Two suggestions for more exhaustive testing, but I think this looks good. Comment at: test/SemaCXX/warn-bitfield-enum-conversion.cpp:1 +// RUN:

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-13 Thread Reid Kleckner via Phabricator via cfe-commits
rnk updated this revision to Diff 91653. rnk marked an inline comment as done. rnk added a comment. - Actually make this warning off by default https://reviews.llvm.org/D30923 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-13 Thread Reid Kleckner via Phabricator via cfe-commits
rnk marked an inline comment as done. rnk added inline comments. Comment at: test/Sema/warn-bitfield-enum-conversion.c:3 + +enum TwoBits { Hi1 = 3 } two_bits; +enum TwoBitsSigned { Lo2 = -2, Hi2 = 1 } two_bits_signed; thakis wrote: > can you add an enum with an

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-13 Thread Reid Kleckner via Phabricator via cfe-commits
rnk updated this revision to Diff 91652. rnk added a comment. - Make test C++, add fixed type enum https://reviews.llvm.org/D30923 Files: include/clang/Basic/DiagnosticGroups.td include/clang/Basic/DiagnosticSemaKinds.td lib/Sema/SemaChecking.cpp

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-13 Thread Nico Weber via Phabricator via cfe-commits
thakis added inline comments. Comment at: test/Sema/warn-bitfield-enum-conversion.c:3 + +enum TwoBits { Hi1 = 3 } two_bits; +enum TwoBitsSigned { Lo2 = -2, Hi2 = 1 } two_bits_signed; can you add an enum with an explicit underlying type? will this warning always

[PATCH] D30923: Warn on enum assignment to bitfields that can't fit all values

2017-03-13 Thread Reid Kleckner via Phabricator via cfe-commits
rnk created this revision. This adds -Wbitfield-enum-conversion, which warns on implicit conversions that happen on bitfield assignment that change the value of some enumerators. Values of enum type typically take on a very small range of values, so they are frequently stored in bitfields.