Patchwork [Fortran] PR 48831 - Constant expression (PARAMETER array element) rejected as nonconstant

login
register
mail settings
Submitter Alessandro Fanfarillo
Date June 3, 2012, 9:41 a.m.
Message ID <CAHqFgjVYvGsktb28-Y8ah6nLHWQ5Esjha9QczmyYB81CWCtOBA@mail.gmail.com>
Download mbox | patch
Permalink /patch/162487/
State New
Headers show

Comments

Alessandro Fanfarillo - June 3, 2012, 9:41 a.m.
Hi all,

in attachment the patch which includes the review comments provided by Tobias.

The patch is bootstrapped and tested on x86_64-unknown-linux-gnu.

Regards.

Alessandro

2012/5/20 Tobias Burnus <burnus@net-b.de>:
> Hi Alessandro,
>
>
> Alessandro Fanfarillo wrote:
>>
>> in attachment there's a patch for PR 48831, it also includes a new
>> test case suggested by Tobias Burnus.
>> The patch is bootstrapped and tested on x86_64-unknown-linux-gnu.
>
>
> Please try to ensure that your patch has a text mime type - it shows up as
>  Content-Type: application/octet-stream;
> which makes reading, reviewing and quoting your patch more difficult.
>
>        PR fortran/48831
>        * gfortran.h: Add non-static prototype declaration
>        of check_init_expr function.
>        * check.c (kind_check): Change if condition related
>        to check_init_expr.
>        * expr.c: Remove prototype declaration of
>        check_init_expr function and static keyword.
>
>
> You should add the name of the function you change in parentheses, e.g.
>
>    * gfortran.h (check_init_expr): Add prototype declaration of function.
>
> (The "non-static" is superfluous as static functions shouldn't be in header
> files.) For "check_init_expr" I'd use "Remove forward declaration" instead
> of "Remove prototype declaration" but that's personal style. But again, you
> should include the function name in parentheses. The reason is that one can
> more quickly find it, if it is always at the same spot.
>
> As mentioned before, the gfortran convention is to prefix functions (gfc_) -
> at least those which are nonstatic. Please change the function name.
>
> -  if (k->expr_type != EXPR_CONSTANT)
> +  if (check_init_expr(k) != SUCCESS)
>
>
> GNU style: Add a space before the "(" of the function argument:
> "check_init_expr (k)".
>
>
> +/* Check an intrinsic arithmetic operation to see if it is consistent
> +   with some type of expression.  */
> +gfc_try check_init_expr (gfc_expr *);
>
>
>
> I have to admit that after reading only the comment, I had no idea what the
> function does - especially the "some type" is not really helpful. How about
> a simple "Check whether an expression is an initialization/constant
> expression."  Initialization and constant expressions are well defined in
> the Fortran standard. (Actually, I find the function name speaks already for
> itself, thus, I do not see the need for a comment, but I also do not mind a
> comment.)
>
> (One problem with the name "constant expression" vs. "initialization
> expression" is that Fortran 90/95 distinguish between them while Fortran
> 2003/2008 have merged them to a single type of expression; Fortran 2003
> calls the merged expression type "initialization expression" while Fortran
> 2008 calls them "constant expressions". In principle, gfortran should make
> the distinction with -std=f95 and reject expressions which are nonconstant
> and only initexpressions when the standard demands it, but I am not sure
> whether gfortran does. That part of gfortran is a bit unclean and the
> distinction between init/const expr is nowadays largely ignored by the
> gfortran developers.)
>
> Otherwise, the patch looks OK.
>
> Tobias
2012-06-03  Alessandro Fanfarillo  <fanfarillo.gcc@gmail.com>
	    Tobias Burnus  <burnus@net-b.de>

	PR fortran/48831
	* gfortran.h (check_init_expr): Add prototype declaration 
	of function.
	* check.c (kind_check): Change if condition related
	to check_init_expr.
	* expr.c (check_init_expr): Remove forward declaration
	and static keyword. Change name in gfc_check_init_expr.

2012-06-03  Alessandro Fanfarillo  <fanfarillo.gcc@gmail.com>

	PR fortran/48831
	* gfortran.dg/parameter_array_element_2.f90: New.

Patch

Index: gcc/fortran/expr.c
===================================================================
--- gcc/fortran/expr.c	(revisione 188147)
+++ gcc/fortran/expr.c	(copia locale)
@@ -1943,12 +1943,6 @@ 
 }
 
 
-/* Check an intrinsic arithmetic operation to see if it is consistent
-   with some type of expression.  */
-
-static gfc_try check_init_expr (gfc_expr *);
-
-
 /* Scalarize an expression for an elemental intrinsic call.  */
 
 static gfc_try
@@ -1994,7 +1988,7 @@ 
   for (; a; a = a->next)
     {
       /* Check that this is OK for an initialization expression.  */
-      if (a->expr && check_init_expr (a->expr) == FAILURE)
+      if (a->expr && gfc_check_init_expr (a->expr) == FAILURE)
 	goto cleanup;
 
       rank[n] = 0;
@@ -2231,7 +2225,7 @@ 
   gfc_actual_arglist *ap;
 
   for (ap = e->value.function.actual; ap; ap = ap->next)
-    if (check_init_expr (ap->expr) == FAILURE)
+    if (gfc_check_init_expr (ap->expr) == FAILURE)
       return MATCH_ERROR;
 
   return MATCH_YES;
@@ -2319,7 +2313,7 @@ 
 			&ap->expr->where);
 	      return MATCH_ERROR;
 	  }
-	else if (not_restricted && check_init_expr (ap->expr) == FAILURE)
+	else if (not_restricted && gfc_check_init_expr (ap->expr) == FAILURE)
 	  return MATCH_ERROR;
 
 	if (not_restricted == 0
@@ -2437,8 +2431,8 @@ 
    intrinsics in the context of initialization expressions.  If
    FAILURE is returned an error message has been generated.  */
 
-static gfc_try
-check_init_expr (gfc_expr *e)
+gfc_try
+gfc_check_init_expr (gfc_expr *e)
 {
   match m;
   gfc_try t;
@@ -2449,7 +2443,7 @@ 
   switch (e->expr_type)
     {
     case EXPR_OP:
-      t = check_intrinsic_op (e, check_init_expr);
+      t = check_intrinsic_op (e, gfc_check_init_expr);
       if (t == SUCCESS)
 	t = gfc_simplify_expr (e, 0);
 
@@ -2573,11 +2567,11 @@ 
       break;
 
     case EXPR_SUBSTRING:
-      t = check_init_expr (e->ref->u.ss.start);
+      t = gfc_check_init_expr (e->ref->u.ss.start);
       if (t == FAILURE)
 	break;
 
-      t = check_init_expr (e->ref->u.ss.end);
+      t = gfc_check_init_expr (e->ref->u.ss.end);
       if (t == SUCCESS)
 	t = gfc_simplify_expr (e, 0);
 
@@ -2592,14 +2586,14 @@ 
       if (t == FAILURE)
 	break;
 
-      t = gfc_check_constructor (e, check_init_expr);
+      t = gfc_check_constructor (e, gfc_check_init_expr);
       if (t == FAILURE)
 	break;
 
       break;
 
     case EXPR_ARRAY:
-      t = gfc_check_constructor (e, check_init_expr);
+      t = gfc_check_constructor (e, gfc_check_init_expr);
       if (t == FAILURE)
 	break;
 
@@ -2629,7 +2623,7 @@ 
   gfc_init_expr_flag = true;
   t = gfc_resolve_expr (expr);
   if (t == SUCCESS)
-    t = check_init_expr (expr);
+    t = gfc_check_init_expr (expr);
   gfc_init_expr_flag = false;
 
   if (t == FAILURE)
Index: gcc/fortran/check.c
===================================================================
--- gcc/fortran/check.c	(revisione 188147)
+++ gcc/fortran/check.c	(copia locale)
@@ -163,7 +163,7 @@ 
   if (scalar_check (k, n) == FAILURE)
     return FAILURE;
 
-  if (k->expr_type != EXPR_CONSTANT)
+  if (gfc_check_init_expr (k) != SUCCESS)
     {
       gfc_error ("'%s' argument of '%s' intrinsic at %L must be a constant",
 		 gfc_current_intrinsic_arg[n]->name, gfc_current_intrinsic,
Index: gcc/fortran/gfortran.h
===================================================================
--- gcc/fortran/gfortran.h	(revisione 188147)
+++ gcc/fortran/gfortran.h	(copia locale)
@@ -2708,6 +2708,7 @@ 
 const char *gfc_extract_int (gfc_expr *, int *);
 bool is_subref_array (gfc_expr *);
 bool gfc_is_simply_contiguous (gfc_expr *, bool);
+gfc_try gfc_check_init_expr (gfc_expr *);
 
 gfc_expr *gfc_build_conversion (gfc_expr *);
 void gfc_free_ref_list (gfc_ref *);
Index: gcc/testsuite/gfortran.dg/parameter_array_element_2.f90
===================================================================
--- gcc/testsuite/gfortran.dg/parameter_array_element_2.f90	(revisione 0)
+++ gcc/testsuite/gfortran.dg/parameter_array_element_2.f90	(revisione 0)
@@ -0,0 +1,16 @@ 
+! { dg-do compile }
+! PR fortran/48831
+! Contributed by Tobias Burnus
+
+program p1
+    implicit none
+    integer, parameter  :: i1    = kind(0)
+    integer, parameter  :: i2(1) = [i1]
+    integer(kind=i2(1)) :: i3
+
+    i3 = int(0, i1)
+    print *, i3
+
+    i3 = int(0, i2(1))  ! This line gives an error when compiling.
+    print *, i3
+end program p1