Re: [patch, fortran] Implement maxloc and minloc for character

[Janne Blomqvist <blomqvist.janne@gmail.com>]

On Tue, Nov 21, 2017 at 9:50 PM, Thomas Koenig <tkoenig@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