This is the mail archive of the gcc-patches@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: PING: [patch, fortran] PR 27997: Implement F2003-style array constructor with typespec


Hi Daniel,

Here is my review of your revised patch. First, a small formal issue: you need to provide updated ChangeLog entries with an updated patch. Second, you need to indicate what testing has been done on the new patch and where (again, something along the lines of "bootstrapped and regtested on x86_64-linux, with both -m32 and -m64). [1] Third, until you have your copyright assignment confirmed, no commit possible, so I propose you send us a final, updated version of your patch after we work on the last comments below and ChangeLog entries, for reference until we can commit it.


[1] For those who might ask, the command-line to run a regtest with both -m32 and -m64 on a 64-bit multilibbed host such as x86_64-linux is:
make check-gfortran RUNTESTFLAGS="--target_board=unix'{-m32,-m64}'" ${*}




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

For this I didn't do anything by now, if it is ok I suggest we look at this separatly?

OK. But please give it some thought if you have a bit of time.


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).

Renamed the variable to typespec_chararray_ctor; is this what you meant?

OK.


+        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?

Removed the braces (and the use of mpz_get_si here, too). For the error, I believe this is what you talked about with Tobias, right (i.e., no need as reducing length is valid)?

Yep, I was mistaken to suggest an error.


+ /* get_array_ctor_strlen walks the elements of the constructor, if a
+ typespec was given, we already know the string length and want the one
+ specified there. */
+ /* XXX: Adding typespec_chararray_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? */
+ if (typespec_chararray_ctor && ss->expr->ts.cl->length
+ && ss->expr->ts.cl->length->expr_type != EXPR_CONSTANT)


I'd prefer we find out and don't leave such a comment (and question) in the code: "adding" will not mean anything to the reader when the rest of the code has changed, and now is the best time to solve this issue. What is the backtrace of the ICE if you leave typespec_chararray_ctor out?

FX

--
François-Xavier Coudert
http://www.homepages.ucl.ac.uk/~uccafco/


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