On Wed, Jul 8, 2020 at 10:34 AM Alex Elder <elder@xxxxxxxxxx> 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. > >> 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: 1. 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? (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) -- Thanks, ~Nick Desaulniers
