Hi Andre,
Thanks for the review. I have partially responded to your comments - see below.
Committed as revision 241450.
Cheers
Paul
On 23 October 2016 at 14:45, Andre Vehreschild <ve...@gmx.de> wrote:
> Hi Paul,
>
> here are my comments to your patch:
>
>> Index: gcc/fortran/class.c
>> ===================================================================
>> *** gcc/fortran/class.c (revision 241439)
>> --- gcc/fortran/class.c (working copy)
>> *************** add_procs_to_declared_vtab (gfc_symbol *
>> --- 2187,2219 ----
>> gfc_symbol *
>> gfc_find_derived_vtab (gfc_symbol *derived)
>> {
>> ! gfc_namespace *ns = NULL;
>
> Setting this to NULL for consistency?
Indeed - =NULL eliminated. It was needed in one incarnation of the patch.
>
>> Index: gcc/fortran/dump-parse-tree.c
>> ===================================================================
>> *** gcc/fortran/dump-parse-tree.c (revision 241439)
>> --- gcc/fortran/dump-parse-tree.c (working copy)
>> *************** show_code_node (int level, gfc_code *c)
>> *** 1843,1848 ****
>> --- 1843,1877 ----
>
> Well, the code in this chunk is identical to the one of EXEC_SELECT, besides
> two lines where the statement's name is printed. I propose to do something
> like:
>
> case EXEC_SELECT:
> case EXEC_SELECT_TYPE:
> d= ..
> fputs ("SELECT", dumpfile);
> if (c->op == EXEC_SELECT_TYPE)
> fputs (" TYPE", dumpfile);
> ...
> // and the same for "END SELECT..."
>
> This would reduce the amount of copied code. An improvement in one
> EXEC_SELECT-dump-handler would then automagically available in the other, too.
Have done this, except for the end, where I have retained "END SELECT" for both.
>
>> Index: gcc/fortran/resolve.c
>> ===================================================================
>> *** gcc/fortran/resolve.c (revision 241439)
>> --- gcc/fortran/resolve.c (working copy)
> <snipp>
>> *************** resolve_select_type (gfc_code *code, gfc
> <snipp>
>> --- 8595,8641 ----
>> else
>> ns->code->next = new_st;
>> code = new_st;
>> ! code->op = EXEC_SELECT_TYPE;
>>
>> + /* Use the intrinsic LOC function to generate the an integer expression
>> + for the vtable of the selector. Note that the rank of the selector
>> + expression has to be set to zero. */
>
> double article: _the an_ !!!
Corrected - thanks.
>
>> Index: gcc/fortran/trans-stmt.c
>> ===================================================================
>> *** gcc/fortran/trans-stmt.c (revision 241439)
>> --- gcc/fortran/trans-stmt.c (working copy)
>> *************** gfc_trans_do_while (gfc_code * code)
>> *** 2331,2336 ****
>> --- 2331,2455 ----
> <snipp>
>> +
>> + /* Translate an assignment to a CLASS object
>> + (pointer or ordinary assignment). */
>> +
>> +
>
> Here is no routine the above comment could document. Left over from prior
> version?
This is in your tree, not mine :-)
>
>> /* End of prototype trans-class.c */
>
>
>> Index: gcc/fortran/trans-stmt.c
>> ===================================================================
>> *** gcc/fortran/trans-stmt.c (revision 241439)
>> --- gcc/fortran/trans-stmt.c (working copy)
>> *************** gfc_trans_do_while (gfc_code * code)
>> *** 2331,2336 ****
>> --- 2331,2455 ----
>
> Can one optimize this by using the "old style" for intrinsic types, i.e. a
> computed goto (switch-case) for them? And in the default case the if-chain on
> the derived types/classes? Would we gain any speed by this? What is your
> opinion on this?
Maybe this would gain something but I suspect that it would not amount to much.
>
>> Please find attached a revised version of the patch that corrects one
>> or two tiny wrinkles. I have removed the tidy up of vtable retrieval
>
> I haven't understood yet, what you need to do for this. Looking forward to
> that
> patch.
OK will submit this separately.
>
> With the above small changes the patch is ok for trunk given that Dominique
> doesn't find any issues.
>
> Beware, that my big patch on polymorphic assignment will *not* be backported
> to gcc-6. I.e., this version of your patch will most probably not be
> applyable.
> You rather will need to apply the old version.
>
> Thanks for the work.
>
> Regards,
> Andre
>
>> Functionally, the patch is as described in the original submission.
>>
>> As attached, it bootstraps and regtests on FC21/x86_64. OK for trunk
>> and, after a decent interval for 6-branch?
>>
>> Cheers
>>
>> Paul
>>
>> 2016-10-22 Paul Thomas <pa...@gcc.gnu.org>
>>
>> PR fortran/69834
>> * class.c (gfc_find_derived_vtab): Obtain the gsymbol for the
>> derived type's module. If the gsymbol is present and the top
>> level namespace corresponds to a module, use the gsymbol name
>> space. In the search to see if the vtable exists, try the gsym
>> namespace first.
>> * dump-parse-tree (show_code_node): Add explicit dump for the
>> select type construct.
>> * resolve.c (build_loc_call): New function.
>> (resolve_select_type): Add check for repeated type is cases.
>> Retain selector expression and use it later instead of expr1.
>> Exclude deferred length TYPE IS cases and emit error message.
>> Store the address for the vtable in the 'low' expression and
>> the hash value in the 'high' expression, for each case. Do not
>> call resolve_select.
>> * trans.c(trans_code) : Call gfc_trans_select_type.
>> * trans-stmt.c (gfc_trans_select_type_cases): New function.
>> (gfc_trans_select_type): New function.
>> * trans-stmt.h : Add prototype for gfc_trans_select_type.
>>
>> 2016-10-22 Paul Thomas <pa...@gcc.gnu.org>
>>
>> PR fortran/69834
>> * gfortran.dg/select_type_1.f03: Change error for overlapping
>> TYPE IS cases.
>> * gfortran.dg/select_type_36.f03: New test.
>
>
> --
> Andre Vehreschild * Email: vehre ad gmx dot de
--
The difference between genius and stupidity is; genius has its limits.
Albert Einstein
