Hello Thomas,
below a very timely review - your patch is not even a month old
and was never pinged, besides, you have chosen an unlucky day.
(In other words: Sorry for the slow review.)
Thomas Koenig wrote on Fri, 13 Apr 2012:
> this patch replaces a != '' with len_trim(a) != 0, to
> speed up the comparison.
I wonder how much it helps - especially for the real world
code. Let's see whether the bug reporter will report back.
Can you also check kind=4 string in the test case?
I think your patch should simply work, but having a test
surely cannot harm.
> + /* Only use new-style comparisions. */
> + switch(op)
> + {
> + case INTRINSIC_EQ_OS:
> + op = INTRINSIC_EQ;
> + break;
I have to admit that I do not like that part. At least for
this patch, I think it neither makes the code clearer nor
shorter. The only hypothetical advantage I see is that it
avoids some issues related to forgetting the _OS version in
the switch statements. Thus, my answer whether I like the
change is a (very weak) NO. But my answer to whether you
may do the change is YES.
> }
>
> +/* Return true if a constant string contains spaces only. */
Nit: Missing line break. (Two empty lines separate functions.)
I would use "only spaces" or even "only blanks" instead of
"spaces only".
> + for (i=0; i<e->value.character.length; i++)
Missing space around the "<", i.e. "i < e->value".
> +}
> +
> +/* Insert a call to the intrinsic len_trim. Use a different name for
Empty line missing.
> +}
> +
> /* Optimize expressions for equality. */
Ditto.
> + gfc_get_sym_tree("__internal_len_trim", current_ns, &fcn->symtree, false);
Blank missing before "(".
> + {
> +
> + bool empty_op1, empty_op2;
Spurious empty line.
> + empty_op1 = empty_string(op1);
> + empty_op2 = empty_string(op2);
Blank missing before "(".
Otherwise, the patch is OK.
Tobias
