[PATCH 4/8] BFD: Add BFD pointer member to `struct carsym'

Maciej W. Rozycki macro@redhat.com
Fri Oct 24 13:37:26 GMT 2025


On Mon, 20 Oct 2025, Jan Beulich wrote:

> >>> +.{* Holds a file position or bfd* depending on context.  *}
> >>> +.typedef union file_ptr_or_bfd
> >>> +.{
> >>> +.  file_ptr file_offset;
> >>> +.  bfd *abfd;
> >>> +.}
> >>> +.file_ptr_or_bfd;
> >>
> >> The comment is too little for my taste, I'm afraid. How would one know which
> >> of the two it is, merely seeing an instance of this type?
> > 
> >  It's up to the user of this data structure to decide, just as the comment 
> > says.  Can you please elaborate what kind of description you'd like to see 
> > here?
> 
> Hard to make a suggestion without knowing what, in at least an example
> case, the approach to distinguish is. To take an abstract example: There
> may be callback functions which want to take either kind of context data.
> There it would be clear that the "scope" is strictly that between the
> site setting up the data and the callback function. Since here you embed
> the new type in another struct, it's less (if at all) clear.

 Please let's not invent hypothetical use cases.  This is a generic data 
type and it doesn't set the usage policy.  It's up to the code writer to 
use this data type in a reasonable manner and you can't figure out which 
of the two members to use from the data type alone.  It's not uncommon for 
code to use such data types.  The comment is as accurate as it can be in 
my opinion.

 I guess I could rewrite the comment to say:

"Holds an archive member reference via a file position or bfd* depending 
on context."

but it wouldn't fulfil your criteria and would dictate usage unecessarily, 
so I'm skeptical as to such an "improvement".

 FWIW one use case of this data type is coming with 7/8, where the member 
to use is determined by bfd_ardata (abfd)->symdef_use_bfd.  Another use 
case will come with the second part of this effort.

 Actually I have realised this data entity gratuitously uses a signed data 
type for the file offset while in similar places we have an unsigned one, 
so I'll respin with this change preceded by one taking care of signedness. 
It might make sense to do a larger cleanup in this area, for example I've 
identified places where ufile_ptr data is assigned to by strtol/strtoll 
rather than strtoul/strtoull, but I won't do this on this occasion.

 Thank you for your input anyway.

  Maciej



More information about the Binutils mailing list