lkml.org 
[lkml]   [2020]   [Jul]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] bitfield.h: don't compile-time validate _val in FIELD_FIT
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.

> >> 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

\
 
 \ /
  Last update: 2020-07-08 19:58    [W:0.051 / U:0.268 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site