[PATCH] D23035: [Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros.

[Chandler Carruth via llvm-commits]

On Mon, Aug 1, 2016 at 8:18 PM Justin Lebar via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> jlebar added a comment.
>
> > You only end up using LLVM_CHECKED_BITFIELD_ASSIGN four times in that
> patch, so I'm not sure it's worth factoring the assertion out into a macro
> is worth it.
>
>
> And we only declare three non-single-bit bitfields -- that's a 1.3 calls
> per field!  :)
>
> Joking aside, a corollary of your point of strikes me as an argument
> against wrapping the bitfield's fields: Most bitfields have size 1 [1].
> This idiom would force us to wrap all of them in getters/setters, or
> otherwise we'd have an inconsistent way of accessing the fields, even
> though we probably don't care about wrapping the size-one fields.  With the
> macro, all reads work the same for all widths with the macro without
> requiring you to write five lines of boilerplate per field (when, like you
> point out, you only really want it for a few of the fields).
>

FWIW, I'm much happier with the size one bitfields being wrapped and using
a 'bool' interface if they're used widely. We have a *lot* more warnings
and other helpful features for bool than 'unsigned' that happens to be 1
bit.


>
> Similarly, if you have a bitfield struct with all size-one fields and then
> want to add a size-two field, the idiom would force you to rewrite all
> users.
>
> > Either way we have an education problem: either remember this library
> thing, or remember that round-tripping through the bitfield is the least
> fragile way to range check it.
>
>
> I guess...remembering that a library exists isn't quite the same as
> remembering how to do it (and convincing your reviewers that you're right,
> and so on), though.
>
> I don't want this to come across as though I'm totally sold on the macro.
> An option would be simply to get rid of the range checks in SelectionDAG,
> under the assumption that, if you add a value to one of these enums and
> then use it in SelectionDAG, hopefully you have a test that fails.
>
> [1] Source: Perusing  `git grep ' : [0-9]\+\>'  | grep -v '\?' | grep -v
> test`
>
>
> https://reviews.llvm.org/D23035
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160802/b1b90705/attachment.html>