[PATCH v4 02/22] gas: move code for object attribute parsing into attr.c

[Jan Beulich]

On 04.07.2025 15:55, Matthieu Longo wrote:
> On 2025-07-04 10:18, Jan Beulich wrote:
>> On 03.07.2025 18:27, Matthieu Longo wrote:
>>> ---
>>
>> Please can you try to get used to having non-empty descriptions unless a
>> patch is really small and straightforward and doesn't leave open the
>> possibility for someone asking why a certain change is being done? (It's
>> really the "why" here that imo would deserve a sentence or two.)
>>
>>>   gas/Makefile.am      |   2 +
>>>   gas/Makefile.in      |  11 +-
>>>   gas/attr.c           | 232 +++++++++++++++++++++++++++++++++++++++++++
>>>   gas/attr.h           |  33 ++++++
>>>   gas/config/obj-elf.c | 210 +--------------------------------------
>>>   gas/config/obj-elf.h |   4 +-
>>>   gas/po/POTFILES.in   |   2 +
>>>   7 files changed, 278 insertions(+), 216 deletions(-)
>>>   create mode 100644 gas/attr.c
>>>   create mode 100644 gas/attr.h
>>
>> Seeing where the code moves from, and (according to an answer to an earlier
>> question I raised) there being no plans to extend this attribute machinery to
>> non-ELF, the files probably want naming elf-attr.[ch] or elf-obj-attr.[ch].
>> And at least the .c file's contents wants to be guarded by an OBJ_ELF /
>> OBJ_MAYBE_ELF conditional (to avoid adding entirely dead code to assemblers
>> for non-ELF targets).
> 
> Disclaimer: I will have a look at others comments later.
> 
> Regarding this one particularly, Richard Earnshaw suggested to me to 
> move those files into gas/config. It does not change the need for 
> #ifdefery inside the header to disable the prototypes, and in others 
> parts of the code using the API.
> 
> Here is a draft of what it look like. You can apply the patch on top of 
> the latest commit of the patch series.
> 
> It seems to me that this mechanism to include/exclude files is more 
> adapted to a target which adds a few specific files in order to split 
> its target code logic into different files, but those files should be 
> unused by others targets.
> 
> What do you think of this approach ?
> Should I keep the files in ./gas or move them to ./gas/config ?

I wouldn't strictly mind either (which is why I didn't comment in this
direction), but I prefer the placement in gas/config/. Whether
obj-elf-attr.[ch] is a good name I can't really judge (all of the
naming long predates my involvement in binutils); as was mentioned in
earlier discussion, obj-*.[ch] generally have a fixed meaning there.
Then again the new file merely is a "branch office" of obj-elf.[ch],
and hence it may well be okay to keep that name.

Jan