[PATCH] D23035: [Support] Add LLVM_BITFIELD_WIDTH and LLVM_CHECKED_BITFIELD_ASSIGN macros.
Reid Kleckner via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 1 17:36:24 PDT 2016
rnk added a comment.
In https://reviews.llvm.org/D23035#502871, @jlebar wrote:
> > I think we should just encourage this pattern
>
>
> Do you think it would be an improvement to rewrite clang's Expr.h to this pattern? Or, more to my point, https://reviews.llvm.org/D23036? It seems kind of wordy to me, but I'm open to thoughts.
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.
> I don't like that this macro obfuscates the code, but I do like that we don't have to rely on people writing correct check-in-range code. Like, it wasn't at all obvious to me that `foo.x = y; assert(foo.x == y);` was correct, or that it *wouldn't* be correct if the bitfield were signed. The macro gets that stuff right, which I like.
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. It's a *little* verbose, but there's enough less magic that I like writing it out a bit more.
https://reviews.llvm.org/D23035
More information about the llvm-commits
mailing list