Re: [PATCH net-next v3 1/3] hexdump: Implement macro for converting large buffers

[David Laight]

On Fri, 21 Feb 2025 12:50:59 -0600
Nick Child <nnac123@xxxxxxxxxxxxx> wrote:

> On Fri, Feb 21, 2025 at 06:04:35PM +0000, David Laight wrote:
> > On Fri, 21 Feb 2025 11:37:46 -0600
> > Nick Child <nnac123@xxxxxxxxxxxxx> wrote:  
> > > On Thu, Feb 20, 2025 at 10:00:50PM +0000, David Laight wrote:  
> > > > You could do:
> > > > #define for_each_line_in_hex_dump(buf_offset, rowsize, linebuf, linebuflen, groupsize, buf, len, ascii) \
> > > > for (unsigned int _offset = 0, _rowsize = (rowsize), _len = (len); \
> > > > 	((offset) = _offset) < _len && (hex_dump_to_buffer((const char *)(buf) + _offset, _len - _offset, \  
> >           ^ needs to be buf_offset.
> >   
> > > > 		_rowsize, (groupsize), (linebuf), (linebuflen), (ascii)), 1); \
> > > > 	_offset += _rowsize )
> > > > 
> > > > (Assuming I've not mistyped it.)
> > > >     
> > >
> > > Trying to understand the reasoning for declaring new tmp variables;
> > > Is this to prevent the values from changing in the body of the loop?  
> >
> > No, it is to prevent side-effects happening more than once.
> > Think about what would happen if someone passed 'foo -= 4' for len.
> >  
> 
> If we are protecting against those cases then linebuf, linebuflen,
> groupsize and ascii should also be stored into tmp variables since they
> are referenced in the loop conditional every iteration.
> At which point the loop becomes too messy IMO.
> Are any other for_each implementations taking these precautions?

No, it only matters if they appear in the text expansion of the #define
more than once.
It may be unlikely here, but for things like min(a,b) where:
#define min(a, b) ((a) < (b) ? (a) : (b))
causes:
	a = 3;
	b = min(a += 3, 7);
to set b to 9 it has to be avoided.

> 
> Not trying to come off dismissive, I genuinely appreciate all the
> insight, trying to learn more for next time.
> 
> > > I tried to avoid declaring new vars in this design because I thought it
> > > would recive pushback due to possible name collision and variable
> > > declaration inside for loop initializer.  
> >
> > The c std level got upped recently to allow declarations inside loops.
> > Usually for a 'loop iterator' - but I think you needed that to be
> > exposed outsize the loop.
> > (Otherwise you don't need _offset and buf_offset.
> >  
> 
> As in decrementing _len and increasing a _buf var rather than tracking
> offset?
> I don't really care for exposing the offset, during design I figured
> some caller may make use of it but I think it is worth removing to reduce
> the number of arguments.

Except the loop body needs it - so it needs to be a caller-defined name,
even if they don't declare the variable.

	David

> 
> Thanks again,
> Nick