This is the mail archive of the gcc@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH] warning about const multidimensional array as function parameter


On Sat, 25 Oct 2014, Martin Uecker wrote:

> Strictly speaking the C standard considers such pointers to be
> incompatible. This seems to be an unintentional consequence 
> of how qualifiers are always attached to the element type. 
> (I am trying to get the standard revised too.) The new 
> behaviour should also be more compatible with C++.

What is the exact difference in wording in the C++ standard that results 
in this difference in semantics?  If we implement the C++ semantics in 
some cases for C, we should make sure to be as compatible is possible with 
C++.  (Within the constraints of not introducing extra warnings by 
default, I suppose - a conversion from const int (*)[] to void * is valid 
for C, even if not for C++.  Of course -Wc++-compat could warn for such 
cases valid for C but not for C++, but that would be a separate issue.)

Note that there are several cases affected (assignment / initialization / 
function call / return; conditional expressions; pointer subtraction; 
pointer comparisons (ordered and unordered), at least) so we should make 
sure of what the standard wording covers, and make sure that all relevant 
cases are covered in the testsuite (both for getting the correct -pedantic 
diagnostic, and not getting the diagnostic in the default case if that's 
what's intended).  These cases may also apply to both single- and 
multi-dimensional arrays.  And where a composite type is formed in a 
conditional expression, there's the matter of making sure it has all the 
qualifiers from either side of the expression (I think the existing code 
will get that right, so it's just a matter of covering it in the 
testsuite).

The acceptance of this as an extension should also be documented in 
extend.texi.

> -  /* Do not lose qualifiers on element types of array types that are
> -     pointer targets by taking their TYPE_MAIN_VARIANT.  */
> -  if (TREE_CODE (mvl) != ARRAY_TYPE)
> -    mvl = (TYPE_ATOMIC (mvl)
> -	   ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
> -	   : TYPE_MAIN_VARIANT (mvl));
> -  if (TREE_CODE (mvr) != ARRAY_TYPE)
> -    mvr = (TYPE_ATOMIC (mvr)
> -	   ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
> -	   : TYPE_MAIN_VARIANT (mvr));
> +  /* For -pedantic record result of comptypes on arrays before loosing 
> +     qualifiers on the element type below. */
> +  val2 = 1;
> +
> +  if (TREE_CODE (mvl) == ARRAY_TYPE 
> +        && TREE_CODE (mvr) == ARRAY_TYPE)
> +    val2 = comptypes (mvl, mvr);
> +
> +  /* Qualifiers on element types of array types that are
> +     pointer targets are lost by taking their TYPE_MAIN_VARIANT.  */
> +
> +  mvl = (TYPE_ATOMIC (mvl)
> +	 ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl), TYPE_QUAL_ATOMIC)
> +	 : TYPE_MAIN_VARIANT (mvl));
> +
> +  mvr = (TYPE_ATOMIC (mvr)
> +	 ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvr), TYPE_QUAL_ATOMIC)
> +	 : TYPE_MAIN_VARIANT (mvr));

Won't this remove _Atomic even on arrays (since it's the element, possibly 
after multiple levels of indirection, that satisfies TYPE_ATOMIC, rather 
than the array itself)?  As in C standard terms _Atomic types aren't 
qualified (it's syntactically a qualifier, but not generally included in 
the term "qualified type"), I think diagnostics *should* still be given by 
default for e.g. passing int (*)[] where _Atomic int (*)[] is expected.  
(Part of the logic for the incompatibility is that it's allowed for 
_Atomic int to be larger than plain int, for example.)

> @@ -5961,7 +5974,7 @@ convert_for_assignment (location_t locat
>        int target_cmp = 0;   /* Cache comp_target_types () result.  */
>        addr_space_t asl;
>        addr_space_t asr;
> -
> +	
>        if (TREE_CODE (mvl) != ARRAY_TYPE)
>  	mvl = (TYPE_ATOMIC (mvl)
>  	       ? c_build_qualified_type (TYPE_MAIN_VARIANT (mvl),

Please avoid spurious diff hunks (especially where the change is in the 
wrong direction - adding trailing whitespace, as here).

> @@ -6083,6 +6096,16 @@ convert_for_assignment (location_t locat
>  		  == c_common_signed_type (mvr))
>  	      && TYPE_ATOMIC (mvl) == TYPE_ATOMIC (mvr)))
>  	{
> +          /* For arrays consider element type instead. This is required because
> +             we warn about conversions for pointers to arrays with different
> +             qualifiers on the element type only for -pedantic.  */
> +          if (TREE_CODE (ttr) == ARRAY_TYPE
> +              && TREE_CODE (ttl) == ARRAY_TYPE)
> +            {
> +              ttr = TREE_TYPE (ttr);
> +              ttl = TREE_TYPE (ttl);
> +            } 

This doesn't look to me as if it will work for multidimensional arrays.  
Presumably you still want diagnostics for passing const int (*)[1][1][1] 
where int (*)[1][1][1] is expected, but not vice versa.

> Index: gcc/testsuite/gcc.dg/qual-component-1.c
> ===================================================================
> --- gcc/testsuite/gcc.dg/qual-component-1.c	(Revision 216690)
> +++ gcc/testsuite/gcc.dg/qual-component-1.c	(Arbeitskopie)
> @@ -110,24 +110,24 @@ g (void)
>      int (*b)[1] = &v1.b;
>      int (*c)[2][3] = &v1.c;
>      int (*cc)[3] = v1.c;
> -    const int (*ff)[3] = v1.c; /* { dg-warning "initialization from incompatible pointer type" } */
> +    const int (*ff)[3] = v1.c;

No, I think you should add -pedantic to this test instead of removing 
tests for diagnostics from it (of course the particular text of the 
diagnostics being tested for may change, that's fine).

-- 
Joseph S. Myers
joseph@codesourcery.com


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]