On Tue, Nov 21, 2017 at 9:50 PM, Thomas Koenig <tkoe...@netcologne.de> wrote:
> Hi Janne,
>
>
>>> So, any other comments about my patch? OK for trunk?
>>
>>
>> - In many cases the copyright notice has "This file is part of the GNU
>> Fortran 95 runtime library (libgfortran)." It's a while since we've
>> called ourselves "GNU Fortran 95", so just remove the "95".
>
>
> That's what I got for copying over an old version of maxloc
> (when it still didn't have NAN handling) as a basis for my own
> patch. This also meant that I had an old copyright notice. Fixed.
Uh, it seems the patch you posted didn't actually fix this?
>> - It seems in the library you're using int for string lengths? Please
>> use gfc_charlen_type instead (in the frontend, gfc_charlen_type_node).
>> (Most of the charlen->size_t patch is fixing up places where we're
>> accidentally using int instead of gfc_charlen_type..).
>
>
> Fixed.
Not everywhere? At least
zgrep "int len" p8.diff.gz
turns up some cases?
>> - Why are you using GFC_INTEGER_1 / GFC_INTEGER_4 to loop over the
>> arrays rather than char/gfc_char4_t? Not sure if it makes any
>> difference in practice, but it sure seems confusing..
>
>
> The reason has to do with evil m4 magic. I used a macro
> from iparm.m4, atype_code. Changing m4 code should mostly
> be restricted to those cases where it is _really_ necessary
> (people, say, not without justification, that m4 is a write-only
> langauge).
Fair enough. :)
>> - Not really related to your patch, but memcmp_char4 sure looks
>> redundant. Isn't it the same as memcmp(a, b, size*4), in which case we
>> could use optimized memcmp implementations?
>
>
> Big/little endian issues.
>
> Consider the following short program:
>
> #include <stdio.h>
> #include <string.h>
>
> char a[4] = { 1, 2, 3, 4};
> char b[4] = { 4, 3, 2, 1};
>
> int main()
> {
> int i, j;
> memcpy (&i, a, sizeof(i));
> memcpy (&j, b, sizeof(j));
> printf("memcmp : ");
> if (memcmp (&i,&j,sizeof(i)))
> printf("larger\n");
> else
> printf("smaller\n");
>
> printf("Direct comparison: ");
> if (i > j)
> printf("larger\n");
> else
> printf("smaller\n");
>
> return 0;
> }
>
> On my x86_64 system, this prints
>
> memcmp : larger
>
>
> Direct comparison: larger
>
> On a big-endian system, this prints
>
> memcmp : larger
> Direct comparison: smaller
Ooh, indeed.
> However, I just learned about the __BYTE_ORDER__ macro.
> We could use that (and in places where we currently have
> a runtime check, for example in replacing the big_endian
> global variable in libgfortran). But that is for another day.
Yup.
> So, attached is a new version of the patch. No update
> on the ChangeLog. OK for trunk?
Yup, just really fix the copyright and string length stuff first. Thanks!
--
Janne Blomqvist
