I'm someone who really doesn't like it. The U in 1U << 31 is unnecessary visual clutter that really makes the number harder to read (I guess 1u is slightly better but not by that much). I think we should define our code style first and foremost to make code easily written and easily read (with no ambiguity and no risk for issues). This isn't a "real" issue in any way -- both GCC and clang handle this the right way, and they even intentionally provide warning switches to allow this (-Wshift-overflow=1 vs -Wshift-overflow=2). So this is perfectly well supported and there's no indication it ever wouldn't in the future. The only thing that could be said against it is that it's not official standard C, and to that I would say that the official C standard is just dumb and incomplete in general. We already frequently use plenty of GCC extensions throughout coreboot, because the equivalent code can either not be written or not be written as cleanly or safely in fully standard-compliant C. In my book, cleanliness and readability always win against just doing what the standard says for the standard's sake.
I am okay with BIT(), I think that's a fair alternative and I wouldn't mind existing cases being rewritten if someone really wants to do that. But that still leaves the problem of 3 << 30 and things like that, so I don't think it's a full solution. (You can't really expand the macro to the multi-bit case without defining what's essentially just a wrapper macro for the shift operator, and at that point I don't think it makes sense anymore.) One example of things that happen when you insist on 1U is that constants cannot be shared between C and assembly code anymore (because the assembler doesn't support the 1U syntax). If you wanna solve this you need to define a macro that either does or doesn't add the U based on whether it's being built for assembler or C (and I think even that doesn't work for inline assembly?), and then you have something like U(1) << 31 everywhere. You can see how that looks in the Trusted Firmware project which does it that way (e.g. https://review.trustedfirmware.org/plugins/gitiles/TF-A/trusted-firmware-a/+/refs/heads/master/include/arch/aarch32/arch.h#126)... if you ask me it's ugly, harder to read and unnecessarily confusing to new contributors. (Also, slight tangent but maybe this is a good opportunity to shill the new DEFINE_BITFIELD/SET32_BITFIELDS() API we added a year ago: https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/include/device/mmio.h#60 ? By defining bit fields semantically you don't have to deal with raw shifts at all anymore, which is both safer and in my opinion more readable. It already has unsigned casts wrapped in the macros so there's no reason to add a U manually anywhere there anymore.) _______________________________________________ coreboot mailing list -- coreboot@coreboot.org To unsubscribe send an email to coreboot-le...@coreboot.org
