On Wed, 8 Jul 2020 10:56:43 -0700 Nick Desaulniers wrote: > 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. 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: > 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? 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.
