Messages in this thread | | | Date | Mon, 31 Jul 2023 10:20:37 +0200 | From | Petr Mladek <> | Subject | Re: arm32 build warnings in workqueue.c |
| |
On Fri 2023-06-23 09:47:56, Tejun Heo wrote: > Hello, > > On Fri, Jun 23, 2023 at 11:24:16AM -0700, Linus Torvalds wrote: > ... > > enum { > > [..] > > WORK_STRUCT_FLAG_BITS = WORK_STRUCT_COLOR_SHIFT + > > WORK_STRUCT_COLOR_BITS, > > [..] > > WORK_STRUCT_FLAG_MASK = (1UL << WORK_STRUCT_FLAG_BITS) - 1, > > WORK_STRUCT_WQ_DATA_MASK = ~WORK_STRUCT_FLAG_MASK, > > } > > > > Anyway, that code absolutely has to be fixed. Using enums for types is > > wrong, wrong, wrong. You should consider an enum to be a weakly typed > > integer expression with some specific advantages (the automatic > > incrementing at definition time, and the compiler being able to limit > > ranges in switch() statements and maybe doing limited type warnings > > elsewhere) > > The only benefit I care about is it being visible in the type system and > thus in debug info whether that's the usual one or BTF. This makes a huge > difference in tracing, monitoring and debugging. For one off's, it's fine to > track down the define values. For something more sophisticated (e.g. > non-trivial drgn, bcc and other BPF programs), the macro constants are a > source of sadness and a really dumb and insidious one.
Would it help to use const variables?
> > +/* Convenience constants - of type 'unsigned long', not 'enum'! */ > > +#define WORK_OFFQ_CANCELING (1ul << __WORK_OFFQ_CANCELING)
static const WORK_OFFQ_CANCELING = 1ul << __WORK_OFFQ_CANCELING;
It is kind of nasty as well. But it defines the right type and should be visible in debuginfo. Well, it is usable only for local variables.
Best Regards, Petr
| |