[RFC v1 1/2] Merge definitions of array_type_nelts_top()

[Alejandro Colomar]

Hi Richard,

On Mon, Jul 29, 2024 at 10:27:35AM GMT, Richard Biener wrote:
> On Sun, Jul 28, 2024 at 4:16 PM Alejandro Colomar <alx@kernel.org> wrote:
> >
> > There were two identical definitions, and none of them are available
> > where they are needed for implementing _Lengthof().  Merge them, and
> > provide the single definition in gcc/tree.{h,cc}, where it's available
> > for _Lengthof().
> >
> > Signed-off-by: Alejandro Colomar <alx@kernel.org>
> > ---
> >  gcc/cp/cp-tree.h              |  1 -
> >  gcc/cp/tree.cc                | 13 -------------
> >  gcc/rust/backend/rust-tree.cc | 13 -------------
> >  gcc/rust/backend/rust-tree.h  |  2 --
> >  gcc/tree.cc                   | 13 +++++++++++++
> >  gcc/tree.h                    |  1 +
> >  6 files changed, 14 insertions(+), 29 deletions(-)
> >

[...]

> > diff --git a/gcc/tree.cc b/gcc/tree.cc
> > index 2d2d5b6db6e..3b0adb4cd9f 100644
> > --- a/gcc/tree.cc
> > +++ b/gcc/tree.cc
> > @@ -3729,6 +3729,19 @@ array_type_nelts (const_tree type)
> >           ? max
> >           : fold_build2 (MINUS_EXPR, TREE_TYPE (max), max, min));
> >  }
> > +
> > +/* Return, as an INTEGER_CST node, the number of elements for TYPE
> > +   (which is an ARRAY_TYPE).  This counts only elements of the top
> > +   array.  */
> > +
> > +tree
> > +array_type_nelts_top (tree type)
> > +{
> > +  return fold_build2_loc (input_location,
> > +                     PLUS_EXPR, sizetype,
> > +                     array_type_nelts (type),
> > +                     size_one_node);
> > +}
> 
> But this is now extremely confusing API with array_type_nelts above this
> saying
> 
> /* Return, as a tree node, the number of elements for TYPE (which is an
>    ARRAY_TYPE) minus one.  This counts only elements of the top array.  */
> 
> so both are "_top".  And there's build_array_type_nelts that's taking
> the number of elements.
> 
> Can you please rename the existing array_type_nelts to
> array_type_nelts_minus_one?  Then _top could be dropped as well from
> the alternate API  you add.

I wanted to do that, but then I found other functions that are named
similarly, such as build_array_type_nelts(), and thought that I wasn't
sure if all of them should be renamed to _minus_one, or just some.  So
I decided to start without renaming.

But yeah, I think I should rename.  I'll prepare a patch for renaming it
independently of this patch set, and send it to be merged before this
patch set.

> I'll also note since array_type_nelts_top calls the other function and that has
> 
>   /* If they did it with unspecified bounds, then we should have already
>      given an error about it before we got here.  */
>   if (! TYPE_DOMAIN (type))
>     return error_mark_node;
> 
> the function should handle error_mark_node (and pass that down).

Hmmmm, now I understand that (! TYPE_DOMAIN (type))

	$ grep -rn return.array_type_nelts gcc
	gcc/cp/call.cc:12111:    return array_type_nelts_top (c->type);
	gcc/c-family/c-common.cc:4090:  return array_type_nelts_top (type);

	$ sed -n 12102,12119p gcc/cp/call.cc
	/* Return a tree representing the number of elements initialized by the
	   list-initialization C.  The caller must check that C converts to an
	   array type.  */

	static tree
	nelts_initialized_by_list_init (conversion *c)
	{
	  /* If the array we're converting to has a dimension, we'll use that.  */
	  if (TYPE_DOMAIN (c->type))
	    return array_type_nelts_top (c->type);
	  else
	    {
	      /* Otherwise, we look at how many elements the constructor we're
		 initializing from has.  */
	      tree ctor = conv_get_original_expr (c);
	      return size_int (CONSTRUCTOR_NELTS (ctor));
	    }
	}

It seems that would fail when measuring for example

	#define memberof(T, member)  ((T){}.member)

	struct s {
		int x;
		int a[];
	};

	__lengthof__(memberof(struct s, a));

I guess?

	$ cat len.c 
	#include <stdio.h>

	#define memberof(T, member)  ((T){}.member)

	struct s {
		int x;
		int y[];
	};

	int
	main(int argc, char *argv[argc + 1])
	{
		int     a[42];
		size_t  n;

		(void) argv;

		//n = __lengthof__(argv);
		//printf("__lengthof__(argv) == %zu\n", n);

		n = __lengthof__(a);
		printf("lengthof(a):\t %zu\n", n);

		n = __lengthof__(long [99]);
		printf("lengthof(long [99]):\t %zu\n", n);

		n = __lengthof__(short [n - 10]);
		printf("lengthof(short [n - 10]):\t %zu\n", n);

		int  b[n / 2];
		n = __lengthof__(b);
		printf("lengthof(b):\t %zu\n", n);

		n = __lengthof__(memberof(struct s, y));
		printf("lengthof(memberof(struct s, y)):\t %zu\n", n);
	}
	alx@debian:~/tmp/gcc$ /opt/local/gnu/gcc/lengthof/bin/gcc len.c 
	alx@debian:~/tmp/gcc$ ./a.out 
	lengthof(a):	 42
	lengthof(long [99]):	 99
	lengthof(short [n - 10]):	 89
	lengthof(b):	 44
	lengthof(memberof(struct s, y)):	 44

Indeed, it's misbehaving.  I'll have a look at that.  I'll probably have
to add an error similar to sizeof's one:

len.c: In function ‘main’:
len.c:37:19: error: invalid application of ‘sizeof’ to incomplete type ‘int[]’
   37 |         n = sizeof(memberof(struct s, y));
      |                   ^

Thanks!

> 
> Note array_type_nelts returns nelts - 1 because that avoids building
> a new tree node for arrays with lower bound zero.

What does it mean that the lower bound is zero?  I didn't understand
that part.

> 
> Thanks,
> Richard.
> 
> >  /* If arg is static -- a reference to an object in static storage -- then
> >     return the object.  This is not the same as the C meaning of `static'.

Have a lovely day!
Alex

-- 
<https://www.alejandro-colomar.es/>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20240729/117d69e0/attachment.sig>