Re: [C PATCH 3/6] c23: tag compatibility rules for struct and unions

[Joseph Myers <joseph@codesourcery.com>]

On Sat, 26 Aug 2023, Martin Uecker via Gcc-patches wrote:

> 	types (convert_for_assignment): Ingore qualifiers.

"Ignore".

> @@ -1993,6 +1993,24 @@ locate_old_decl (tree decl)
>  	    decl, TREE_TYPE (decl));
>  }
>  
> +static tree
> +previous_tag (tree type)

This function needs a comment documenting its semantics.

> @@ -8651,6 +8672,12 @@ start_struct (location_t loc, enum tree_code code, tree name,
>  
>    if (name != NULL_TREE)
>      ref = lookup_tag (code, name, true, &refloc);
> +
> +  /* For C2X, even if we already have a completed definition,
> +     we do not use it. We will check for consistency later.  */
> +  if (flag_isoc2x && ref && TYPE_SIZE (ref))
> +    ref = NULL_TREE;
> +
>    if (ref && TREE_CODE (ref) == code)
>      {
>        if (TYPE_STUB_DECL (ref))

This comes before the check for nested redefinitions (which are still 
invalid) - so meaning that, if ref is set to NULL_TREE here, the check 
for nested redefinitions won't apply.

You have a testcase for nested redefinitions in a slightly different case 
(where the struct's first definition hasn't finished when the nested 
definition is encountered).  But what about the case where: first, the 
struct gets defined; then, in the same scope, it gets redefined, with the 
redefinition containing a nested redefinition?  I don't see anything here 
to detect that case of nested redefinitions

For enums, note that nested redefinitions include cases where the nesting 
is inside an enum type specifier (currently diagnosed by GCC following an 
ordinary redefinition path, not one for nested definitions).

typedef __SIZE_TYPE__ size_t;
enum e : typeof (sizeof (enum e : size_t { A })) { A };

is invalid because the definitions of enum e are nested, so should be 
diagnosed, and there should be a test that it is.

> @@ -8315,6 +8332,13 @@ digest_init (location_t init_loc, tree type, tree init, tree origtype,
>  	   conversion.  */
>  	inside_init = convert (type, inside_init);
>  
> +      if ((code == RECORD_TYPE || code == UNION_TYPE)
> +	  && !comptypes (TYPE_MAIN_VARIANT (type), TYPE_MAIN_VARIANT (TREE_TYPE (inside_init))))
> +	{
> +	  error_init (init_loc, "invalid initializer %qT %qT", type, TREE_TYPE (inside_init));
> +	  return error_mark_node;
> +	}

I'd expect some words between the two type names, or explaining how they 
relate to the initialization, rather than just two type names in 
succession with no explanation of what's the type of the initializer and 
what's the type of the object being initialized.

> diff --git a/gcc/testsuite/gcc.dg/c2x-tag-1.c b/gcc/testsuite/gcc.dg/c2x-tag-1.c

> +struct r { int a; char b[0]; };

I tend to think tests such as this, involving GNU extensions ([0] arrays), 
should go in gnu23-* tests not c23-* ones.

(I'm currently testing the final C2X -> C23 patch, that renames existing 
tests.  The next revision of this patch series will need updating for the 
renaming in both file names and file contents.)

> +++ b/gcc/testsuite/gcc.dg/c2x-tag-10.c

This is definitely a GNU extensions test (VLAs in structures).

> +++ b/gcc/testsuite/gcc.dg/c2x-tag-4.c

Another GNU extensions test (GNU attributes).

> diff --git a/gcc/testsuite/gcc.dg/c2x-tag-7.c b/gcc/testsuite/gcc.dg/c2x-tag-7.c

Another GNU extensions test (VLAs in structures).

-- 
Joseph S. Myers
joseph@codesourcery.com