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

[Jan Beulich]

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).

> --- /dev/null
> +++ b/gas/attr.c
> @@ -0,0 +1,232 @@
> +/* Object attributes parsing.
> +   Copyright (C) 2025 Free Software Foundation, Inc.
> +
> +   This file is part of GAS, the GNU Assembler.
> +
> +   GAS is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3, or (at your option)
> +   any later version.
> +
> +   GAS is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with GAS; see the file COPYING.  If not, write to the Free
> +   Software Foundation, 51 Franklin Street - Fifth Floor, Boston, MA
> +   02110-1301, USA.  */
> +
> +#include "attr.h"
> +#include "safe-ctype.h"
> +
> +

Because they make patch context less meaningful, I think it would be nice
to avoid the introduction of any double blank lines.

> +#define skip_whitespace(str)  do { if (is_whitespace (*(str))) ++(str); } while (0)

A remark here, despite me understanding that you only move this code:
Having loops around whitespace removal is generally pointless. Either
the scrubber has already collapsed successive whitespace into one blank,
or (if the scrubber is bypassed) input code is required to not use any
successive instances of whitespace.

> +static inline int
> +skip_past_char (char ** str, char c)

Same here, i.e. I understand this is only being moved, but going forward
I think functions producing a boolean result should have a return type
of "bool" and ...

> +{
> +  if (**str == c)
> +    {
> +      (*str)++;
> +      return 0;
> +    }
> +  else
> +    return -1;
> +}

... use true / false.

> --- a/gas/config/obj-elf.c
> +++ b/gas/config/obj-elf.c
> @@ -25,6 +25,7 @@
>  #include "obstack.h"
>  #include "dwarf2dbg.h"
>  #include "ginsn.h"
> +#include "attr.h"

Why is this needed, when you already have ...

> --- a/gas/config/obj-elf.h
> +++ b/gas/config/obj-elf.h
> @@ -205,9 +205,7 @@ extern void obj_elf_vtable_entry (int);
>  extern struct fix * obj_elf_get_vtable_inherit (void);
>  extern struct fix * obj_elf_get_vtable_entry (void);
>  
> -/* Object attributes v1.  */
> -extern bool oav1_attr_seen (obj_attr_vendor, obj_attr_tag_t);
> -extern obj_attr_tag_t obj_attr_v1_process_attribute (obj_attr_vendor);
> +#include "attr.h"

... this?

Jan