Re: [PATCH 1/2] iio: adc: ad4130: Fix comparison of channel setups

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2025-02-12 at 10:01 +0100, Uwe Kleine-König wrote:
> On Thu, Jan 30, 2025 at 08:14:39PM +0200, Andy Shevchenko wrote:
> > On Thu, Jan 30, 2025 at 06:45:01PM +0100, Uwe Kleine-König wrote:
> > 
> > ...
> > 
> > > +	BUILD_BUG_ON(sizeof(*a) !=
> > > +		     sizeof(struct {
> > > +				    unsigned int iout0_val;
> > > +				    unsigned int iout1_val;
> > > +				    unsigned int burnout;
> > > +				    unsigned int pga;
> > > +				    unsigned int fs;
> > > +				    u32 ref_sel;
> > > +				    enum ad4130_filter_mode filter_mode;
> > > +				    bool ref_bufp;
> > > +				    bool ref_bufm;
> > > +			    }));
> > 
> > Is I shuffle the fields (for whatever reason) this may give false positive
> > warnings.
> 
> That's fine in by book. Whenever the struct is changed it's a good
> opportunity to check if the cmp function needs adaption, too.
> 

In practise I think shuffling the fields is unlikely...

> > I think this BUILD_BUG_ON() is unreliable and ugly looking
> > (static_assert() won't help much here either), so on the second though I
> > think
> > it's better to simply add a comments in both places (here and near to the
> > structure definition) to explain that these needs to be in sync.
> 
> The nice thing about BUILD_BUG_ON (or static assert) is that the
> compiler quite reliably enforces the two being in sync. A comment
> doesn't.
> 

Agreed... I guess we could also add a comment on top of the affected struct so
that if someone messes with it, is also aware of this comparison.

- Nuno Sá