On Wed, 8 Jul 2020 10:56:43 -0700 Nick Desaulniers wrote:
On Wed, Jul 8, 2020 at 10:34 AM Alex Elder elder@linaro.org wrote:
I understand why something needs to be done to handle that case. There's fancy macro gymnastics in "bitfield.h" to add convenient build-time checks for usage problems; I just thought there might be something we could do to preserve the checking--even in this case. But figuring that out takes more time than I was willing to spend on it yesterday...
I also find the use of 0U in FIELD_GET sticks out from the use of 0ULL or (0ull) in these macros (hard to notice, but I changed it in my diff to 0ULL). Are there implicit promotion+conversion bugs here? I don't know, but I'd rather not think about it by just using types of the same width and signedness.
TBH I just copied the type from other arguments. It doesn't matter in practice now in this case. I have no preference.
A second comment about this is that it might be nice to break __BF_FIELD_CHECK() into the parts that verify the mask (which could be used by FIELD_FIT() here) and the parts that verify other things.
Like so? Jakub, WDYT? Or do you prefer v1+Alex's suggestion about using `(typeof(_mask))0` in place of 0ULL?
Yes, very much like that! But you could do that as a follow-on instead, so as not to delay or confuse things.
No rush; let's get it right.
So I can think of splitting this into maybe 3 patches, based on feedback:
- there's a bug in compile time validating _val in FIELD_FIT, since
we want to be able to call it at runtime with "bad" values. 2. the FIELD_* macros use constants (0ull, 0ULL, 0U) that don't match typeof(_mask). 3. It might be nice to break up __BF_FIELD_CHECK.
I don't think anyone's raised an objection to 1.
Assuming Jakub is ok with 3, fixing 3 will actually also address 2. So then we don't need 3 patches; only 2. But if we don't do 3 first, then I have to resend a v2 of 1 anyways to address 2 (which was Alex's original feedback).
My above diff was all three in one go, but I don't think it would be unreasonable to break it up into 3 then 1.
If we prefer not to do 3, then I can send a v2 of 1 that addresses the inconsistent use of types, as one or two patches.
Jakub, what is your preference?
I don't see much point in breaking up the checking macro. But even less in arguing either way :)
(Also, noting that I'm sending to David, assuming he'll pick up the patches once we have everyone's buy in? Or is there someone else more appropriate to accept changes to this header? I guess Jakub and David are the listed maintainers for drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c)
Seems reasonable, put [PATCH net] in the subject to make that explicit.