Re: PR libfortran/29568 (subrecord patch)

[Thomas Koenig <Thomas dot Koenig at online dot de>]

Hello world,

based on Jerry's approval, and after some minor changes suggested
mainly by Brooks, I have committed the patch to trunk as revision
119412.

Thanks!

I have attached the version I committed (including the Changelog
entries) for reference.

Brooks wrote:

> I'd suggest making "maximum subrecord length" unambiguously refer to the 
> user-supplied value in the UI, never to MAX_SUBRECORD_LENGTH, and 
> writing this error as "Maximum subrecord length cannot be greater than %d".

Done.

> That, of course, is inappropriate for the "value < 1" case, but I think 
> it makes sense either to put in a separate error message for that, or to 
> note that the only possible value that's less than 1 is 0, since 
> UInteger options never have negative values, and that it might make just 
> as much sense to use -fmax-subrecord-length=0 as a flag that means "as 
> unlimited as possible" in the same way that it means that for line 
> lengths and such, and have that set the value to MAX_SUBRECORD_LENGTH.

I have removed the test for 0.  Now, -fmax-subrecord-length=0 will
silently be treated as a do-nothing option.

> Also, I'm somewhat surprised that there isn't a default value for 
> gfc_option.max_subrecord_length added to gfc_init_options.  As best I 
> can tell, if -fmax-subrecord-length is not given, then it's never 
> initialized at all.

I simply forgoten the initialization.  I had also forgotten it
for my previous -frecord-marker and -fconvert patches (also
attached in this patch).

> >+#define MAX_SUBRECORD_LENGTH ((int) ((1u<<31u) - 9u))
> 
> I don't think this is the clearest place to put this; there are several 
> other hard limits of this sort at lines 58-60 in gfortran.h, and I would 
> think this should go with them.

Definition is moved.  I have also used the constant explicitly,
with a comment explaining the value.

> >+#define GFC_MAX_SUBRECORD_LENGTH (GFC_INTEGER_4_HUGE - 8)
> 
> As a curiousity question: why is this different from the value given in 
> gfortran.h?

The value is the same, the writing is different.  Also changed to
the numerical constant.

> Speaking of which, given that you're doing so much mucking about with 
> records, it's probably worth checking to see if you've accidentally 
> fixed PR 30009 in the process.  :)

I haven't yet (I checked :-) but I have assigned the PR to myself.

> P.S. This wasn't posted to gcc-patches@; you might want to do that when 
> you commit it.

I had noticed that, and forwarded my previous mail to gcc-patches.

Thanks everybody!

	Thomas

Attachment: patch-14
Description: Text document