This is the mail archive of the fortran@gcc.gnu.org mailing list for the GNU Fortran 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: PING: [patch, fortran] PR 27997: Implement F2003-style array constructor with typespec

FX wrote:
Ante-scriptum 1: Any news from you copyright assignment?

As already written, I'm still waiting on the confirmation.

Please find my review below. It's globally a well-written patch, following GNU coding standards and has quite a testcases, which is good and we sometime overlook a bit (writing elaborate testcases covering all cases one can think of is a tiresome exercise). This is not yet OK to commit (although it's fairly close), I suspect you'll want to update it and respond to some of my comments below.

Thanks for the review, I'll comment/correct on the items.

2008-04-07 Daniel Kraft <d@domob.eu>

    PR fortran/27997
    * gfortran.h:  Added field "length_from_typespec" to
    gfc_charlength needed for the patch.
    * array.c:  Added parsing of typespec to
    gfc_match_array_constructor and extended
    gfc_resolve_character_array_constructor to correctly
    pad strings when a constant character length was given
    by a typespec.
    * trans-array.c:  Added code to gfc_trans_array_constructor
    to support explicit but dynamic character lengths.


The correct format includes the names of each function touched:

OK, I see. BTW, should I add these lines to the corresponding ChangeLog-files, too, or should this be done on check-in?

Regarding the test-cases, I'll add some with respect to your discussion with Tobias.

Index: gcc/fortran/trans-array.c
===================================================================
--- gcc/fortran/trans-array.c        (revision 133867)
+++ gcc/fortran/trans-array.c        (working copy)
@@ -960,9 +960,10 @@
 }


-/* Assign an element of an array constructor.  */
+/* Variables needed for bounds-checking.  */
 static bool first_len;
 static tree first_len_val;
+static bool typespec_ctor;

 static void
 gfc_trans_array_ctor_element (stmtblock_t * pblock, tree desc,
@@ -999,7 +1000,7 @@
                  se->string_length,
                  se->expr);
     }
-      if (flag_bounds_check)
+      if (flag_bounds_check && !typespec_ctor)
     {
       if (first_len)
         {

I've tried to trigger that code and could not see it work. I cannot see a runtime error, neither with

  call test ("this is long")
contains
  subroutine test(s)
    character(len=*) :: s
    print *, [ character(len=3) :: s, "foo" ]
  end subroutine test
end

nor with

  call test ("this is long")
contains
  subroutine test(s)
    character(len=*) :: s
    print *, [ s, "foo" ]
  end subroutine test
end

As this was not working previously, I'm not asking you to fix it in this patch :) But, being now our most proficient expert in array constructors, you might have an opinion as to why that doesn't work... Otherwise, please file a PR so it's fixed later.

Hm, are you sure about this? I'm not sure myself at the moment, but I *think* I had to add this to get it work for -fbounds-check. But anyways, then we'll leave it like this, right?

@@ -1681,7 +1682,11 @@
   tree type;
   bool dynamic;

- if (flag_bounds_check && ss->expr->ts.type == BT_CHARACTER)
+ /* Do bounds-checking here and in gfc_trans_array_ctor_element only if no
+ typespec was given for the array constructor. */
+ typespec_ctor = (ss->expr->ts.cl && ss->expr->ts.cl->length_from_typespec);

This means you only ever set typespec_ctor for character arrays. This is fine with me, because that's the information we need, but I suggest giving the variable a name that indicates this fact (lest someone uses it for other type).

So something like typespec_chararray_ctor?


+      /* XXX:  Adding typespec_ctor here was merely a hack to make
+     gfortran.dg/array_constructor_17.f90 work (ICE otherwise).  Is this
+     ok anyway or should I look for another solution?  */

I can't tell, because there is no gfortran.dg/array_constructor_17.f90 testcase in your patch :) It depends on why we had an ICE, but it looks OK as it is.

Ok, I'll for the moment leave this in as-is including the comment.

+      /* XXX:  This works for my tests, but is this the correct way to
+         transform the gfc_expr into a tree?  */

Ah... well, it rather depends... you've got the right function, but I'd suggest here using the variant with a type, gfc_conv_expr_type():

I see, so gfc_conv_expr_type is the thing one should generally use when the type is known? I did find gfc_conv_expr merely by unstructured search in the codebase ;)

+ ss->string_length = length_se.expr;


That one is fine. But, you have created gfc_se, and it might have pre and post blocks (i.e. things that were added before and after the loop, like evaluation of some loop invariant quantities). This is done by:

              gfc_add_block_to_block (&loop->pre, &length_se.pre);
              gfc_add_block_to_block (&loop->post, &length_se.post);

which adds these pre and post blocks to the pre and post blocks of the global loop (passed as a parameter to the function).

I'll do it. But my main concern was on correctly releasing anything that should be after the block and if it's safe to simply extract the .expr-member of length_se. But if this is, I'll gladly leave it like this.

+      if (expr->ts.cl->length->ts.type == BT_INTEGER)
+    {
+      if (expr->ts.cl->length->expr_type == EXPR_CONSTANT)
+        {
+          /* Got a constant character length, pad according to this.  */
+          max_length = mpz_get_si (expr->ts.cl->length->value.integer);
+        }
+    }
+    }

I imagine this is written that way for historical reasons. The proper style would be:

      /* We've got a character length specified.  It should be an integer,
     otherwise an error is signalled elsewhere.  */
      gcc_assert (expr->ts.cl->length);

/* If we've got a constant character length, pad according to this. */
if (expr->ts.cl->length->ts.type == BT_INTEGER
&& expr->ts.cl->length->expr_type == EXPR_CONSTANT)
max_length = mpz_get_si (expr->ts.cl->length->value.integer);

I remember I tried it like this first, but this assertion failed; so I concluded this ==BT_INTEGER test is needed but it is simply ok to ignore the case when it is no integer, as this raises an error anyway.

As for the use of mpz_get_si() directly on the value, it is done too often in currently existing code but is not a good coding practice: the expr->ts.cl->length->value.integer might not fit in a host integer. Please use gfc_extract_int() instead. (It is m project, at some point, to audit all uses of mpz_get_si in the front-end, but I never got around to it; we should also have a function gfc_extract_hwint that returns a HOST_WIDE_INT instead of a gfc_extract_int, for that matter).

Cool, I even like gfc_extract_int much better than doing it directly, I just didn't know for know that there is this function :D

+        if (generated_length || ! cl
+        || (cl->expr_type == EXPR_CONSTANT
+            && cl->ts.type == BT_INTEGER
+            && mpz_get_si (cl->value.integer) < max_length))
+          {
+        gfc_set_constant_character_len (max_length, p->expr, true);
+          }

Unneeded braces. It looks like we could easily emit an error for strings of constant length that are too long, couldn't we?

I put the braces in because it looks better for me than this long condition followed by the statement without braces; but if it's common practice to *never* set braces around single statements, I'll do it like this in the future.

For the emitting the error, I'm not sure if I understand you correctly; or is this the thing you talked about with Tobias, so obsolete?

Cheers,
Daniel

--
Done:     Bar-Sam-Val-Wiz, Dwa-Elf-Hum-Orc, Cha-Law, Fem-Mal
Underway: Ran-Gno-Neu-Fem
To go:    Arc-Cav-Hea-Kni-Mon-Pri-Rog-Tou

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