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

[Justin Lebar via llvm-commits]

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).

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