[PATCH v1 03/14] gas: implement parsing of object attributes v2

[Jan Beulich]

On 24.04.2025 18:27, Matthieu Longo wrote:
> On 2025-03-28 14:24, Jan Beulich wrote:
>> On 21.03.2025 18:14, Matthieu Longo wrote:
>>> --- a/gas/config/obj-elf.c
>>> +++ b/gas/config/obj-elf.c
>>> @@ -41,6 +41,10 @@
>>>   #include "elf/alpha.h"
>>>   #endif
>>>   
>>> +#ifdef TC_AARCH64
>>> +#include "elf/aarch64.h"
>>> +#endif
>>
>> Why is this needed? It's imo bat practice to include arch-specific files here,
>> even if ...
>>
> 
> Removed in the next revision.
> 
>>>   #ifdef TC_MIPS
>>>   #include "elf/mips.h"
>>>   #endif
>>
>> ... context on both sides demonstrates that it was done before.
>>
>> For most of the new code following in this file, I'm again not overly happy to
>> see so much dead code added for non-Arm64. Can't interested tc-*.h files have a
>> #define added controlling whether this code is actually to be compiled?
>>
> 
> I refactored the code in the next revision, and shared the parsing code 
> between OAv1 and OAv2. So TC_* defines should not be necessary anymore.
> 
> There is actually a lot of code added here for object attributes (both 
> v1 and v2). I am wondering if we could not move it to another file: 
> obj-attr.c
> What do you think ?

Generally I like splitting things up into smaller files. Splitting up here
may additionally make it easier to omit the code altogether for targets not
using it. So - yes, please, if it's reasonably possible to arrange for.

>>> +  char* val = xmemdup0 (s, i);
>>> +  arg_out->optional_set = true;
>>> +  arg_out->type = STRING_VALUE;
>>> +  arg_out->value.string = val;
>>
>> STRING_VALUE further up was used for string literals. How come it identifies
>> what looks to be an identifier here? If it's an identifier, why would you not
>> use one of the existing primitives to parse that, e.g. get_symbol_name()?
> 
> get_symbol_name seems to do too much for what I needed, specifically the 
> handling of double quotes.

You could reject the double-quotes case after the function returned? Else
there may be other suitable parsing functions already available? If
absolutely nothing suitable is there, perhaps something wants adding, to
avoid too much open-coding here?

Jan