Messages in this thread | | | Subject | Re: [PATCH v3] bitfield: fix *_encode_bits() | From | Johannes Berg <> | Date | Mon, 18 Jun 2018 22:28:03 +0200 |
| |
> I think would be better to add test cases first, followed by fix. (1 > patch -> 2 patches) > In this case Fixes tag would be only for the fix part and backporting > (if needed) will be much easier.
Can't, unless I introduce a compilation issue in the tests first? That seems weird. But I guess I can do it the other way around.
> > @@ -143,6 +143,7 @@ static __always_inline base type##_get_bits(__##type v, base field) \ > > ____MAKE_OP(le##size,u##size,cpu_to_le##size,le##size##_to_cpu) \ > > ____MAKE_OP(be##size,u##size,cpu_to_be##size,be##size##_to_cpu) \ > > ____MAKE_OP(u##size,u##size,,) > > +____MAKE_OP(u8,u8,,) > > Is this one you need, or it's just for sake of tests?
All three ;-)
We'll probably need it eventually (we do have bytes to take bits out of), for consistency I think we want it, and I wanted to add it to the tests too.
> For me looks like for consistency we may add fake conversion macros > for this, such as > > #define cpu_to_le8(x) x > #define le8_to_cpu(x) x > ... > #undef le8_to_cpu > #undef cpu_to_le8 > > And do in the same way like below > > __MAKE_OP(8)
I disagree with this. I don't see why we should have le8_encode_bits() and be8_encode_bits() and friends, that makes no sense.
> Perhaps > // SPDX... GPL-2.0+
Yeah, I guess I should have that.
> > +/* > > + * Test cases for bitfield helpers. > > + */ > > + > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > + > > +#include <linux/init.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > Either module.h (if we can compile as a module) or just init.h otherwise.
It can be a module ... guess I cargo-culted that from another test.
> > +/* > > + * This should fail compilation: > > + * CHECK_ENC_GET(16, 16, 0x0f00, 0x1000); > > + */ > > Perhaps we need some ifdeffery around this. It would allow you to try > w/o altering the source code. > > #ifdef TEST_BITFIELD_COMPILE > ... > #endif
Yeah, I guess we could do that.
> I guess you rather continue and print a statistics X passed out of Y. > Check how it's done, for example, in other test_* modules. > (test_printf.c comes first to my mind).
I see it's done that way elsewhere, but I don't really see the point. It makes the test code more complex, and if you fail here you'd better fix it, and if you need a few iterations for that it's not really a problem?
johannes
| |