Fwd: Adding UNION/MAP -- Feedback and tips pls!

[Janus Weil <janus at gcc dot gnu dot org>]

Hi Russell,

thanks for sending the patch. But please don't send stuff to me
privately. I'm forwarding it to the gfortran mailing list (and hope
you don't mind).

I'm afraid I will not have any time to look at your patch in the
coming few days, but I hope some of the other gfortraners can give you
some feedback and advice in the meantime ...

Cheers,
Janus



---------- Forwarded message ----------
From: Russell Brennan <RussellJBrennan@gmail.com>
Date: 2013/2/26
Subject: Re: Adding UNION/MAP -- Feedback and tips pls!
To: Janus Weil <janus@gcc.gnu.org>


I've attached a patch and my test file.  The two commented out lines
at the bottom cause failure, so it is clear that I am missing
something(s).  A nudge in the right direction would be greatly
appreciated!  TIA,

Russell

On Sat, Feb 23, 2013 at 8:54 AM, Janus Weil <janus@gcc.gnu.org> wrote:
> Hi Russell,
>
>> Ok, I have added parsers for both UNION and MAP, and I wanted to just
>> throw these out there to get some initial feedback and guidance.
>
> good! In general I would recommend to post your code in the form of a
> patch (i.e. the output of "svn diff"), so that people can easily apply
> it to their working copy and try it out. Also I think it's preferable
> to actually attach a file, instead of showing the patch inline in the
> email (in order to avoid problems with line breaking, indentation,
> etc).
>
> I haven't really looked at the code yet, but might do that later ...
>
>
>> The
>> result of -fdump-parse-tree follows, and doesn't look quite right to
>> me (not that I really know what to look for).
>
> Hard to judge without seeing the test case which the dump belongs to,
> I guess. Could you provide that, please?
>
> Thanks,
> Janus
>
>
>
>
>> ======================================
>>
>> /* Parse a UNION.  */
>> static void
>> parse_union (void)
>> {
>>   indent_level=max(0,indent_level); ++indent_level; show_indent();
>>   printf("In parse_union\n");
>>   bool compiling_union=true, seen_map=false;
>>   gfc_statement statement;
>>   gfc_state_data state_data;
>>
>>   gcc_assert (gfc_current_state () == COMP_DERIVED);
>>   gcc_assert (gfc_current_block ());
>>
>>   accept_statement (ST_UNION);
>>   push_state (&state_data, COMP_UNION, NULL);
>>
>>   gfc_typebound_default_access = ACCESS_PUBLIC;
>>
>>   while (compiling_union)
>>     {
>>       statement = next_statement ();
>>       switch (statement)
>>         {
>>         case ST_MAP:
>>           seen_map = true;
>>           parse_map();
>>           break;
>>
>>         case ST_END_UNION:
>>           compiling_union = false;
>>
>>           if (!seen_map)
>>             gfc_notify_std (GFC_STD_F95_DEL, "UNION definition at %C
>> without MAPs");
>>
>>           accept_statement (ST_END_UNION);
>>           break;
>>
>>         case ST_DATA_DECL:
>>           // Data members must be within MAPs
>>           // Am I handling this correctly?
>>           gfc_error("Component declaration at %C not within MAP");
>>           reject_statement();
>>           break;
>>
>>         case ST_NONE:
>>           unexpected_eof ();
>>
>>         default:
>>           unexpected_statement (statement);
>>           break;
>>         }
>>     }
>>
>>   pop_state ();
>>   --indent_level;
>> }
>>
>>
>> /* Parse a MAP.  */
>> static void
>> parse_map (void)
>> {
>>   indent_level=max(0,indent_level); ++indent_level; show_indent();
>>   printf("In parse_map\n");
>>   bool compiling_map=true, seen_component=false;
>>   gfc_statement statement;
>>   gfc_state_data state_data;
>>
>>   gcc_assert (gfc_current_state () == COMP_UNION);
>>   // TODO: The following fails, why? Should I even be checking this?
>>   //gcc_assert (gfc_current_block ());
>>
>>   accept_statement (ST_MAP);
>>   push_state (&state_data, COMP_MAP, NULL);
>>
>>   gfc_typebound_default_access = ACCESS_PUBLIC;
>>
>>   while (compiling_map)
>>     {
>>       statement = next_statement ();
>>       switch (statement)
>>         {
>>         case ST_DATA_DECL:
>>           seen_component = true;
>>           accept_statement(statement);
>>           break;
>>
>>         case ST_END_MAP:
>>           compiling_map = false;
>>
>>           if (!seen_component)
>>             // TODO: is this the proper way to handle a warning-type situation?
>>             gfc_notify_std (GFC_STD_F95_DEL, "Empty MAP definition at %C");
>>
>>           accept_statement(statement);
>>           break;
>>
>>         case ST_UNION:
>>           parse_union();
>>           seen_component = true;
>>           break;
>>
>>         case ST_NONE:
>>           unexpected_eof ();
>>
>>         default:
>>           unexpected_statement(statement);
>>           break;
>>         }
>>     }
>>
>>   pop_state ();
>>   --indent_level;
>> }
>>
>> ======================================
>> Namespace: A-H: (REAL 4) I-N: (INTEGER 4) O-Z: (REAL 4)
>> procedure name = hello_program
>>   symtree: 'Mytype'      || symbol: 'mytype'
>>     type spec : (UNKNOWN 0)
>>     attributes: (DERIVED )
>>     components: (test_mtype_int (INTEGER 4) ())
>>     hash: 51386734
>>     Procedure bindings:
>>     Operator bindings:
>>   symtree: 'hello_program'|| symbol: 'hello_program'
>>     type spec : (UNKNOWN 0)
>>     attributes: (PROGRAM PUBLIC  SUBROUTINE)
>>   symtree: 'mytype'      || symbol: 'mytype'
>>     type spec : (INTEGER 4)
>>     attributes: (PROCEDURE  FUNCTION IMPLICIT-TYPE)
>>     Generic interfaces: mytype
>>   symtree: 'test_union1_1'|| symbol: 'test_union1_1'
>>     type spec : (INTEGER 4)
>>     attributes: (VARIABLE )
>>   symtree: 'test_union1_2'|| symbol: 'test_union1_2'
>>     type spec : (INTEGER 4)
>>     attributes: (VARIABLE )
>>   symtree: 'test_union2_2'|| symbol: 'test_union2_2'
>>     type spec : (INTEGER 8)
>>     attributes: (VARIABLE )
>>
>>   code:
>>   WRITE UNIT=6 FMT=-1
>>   TRANSFER 'Hello World!'
>>   DT_END
>>
>> On Tue, Feb 19, 2013 at 11:38 AM, Russell Brennan
>> <RussellJBrennan@gmail.com> wrote:
>>>>> I haven't been able to get a grasp on which global variables are key,
>>>>> is there any documentation outlining what the variables are for and
>>>>> how they are used?
>>>>
>>>>
>>>> Hmm, there shouldn't be many global variables, the main ones are
>>>> gfc_current_locus and gfc_current_ns.
>>> I am seeing globals such as new_st, gfc_state_stack, and gfc_new_block
>>> for instance.  Anything that isn't passed between function calls
>>> really.
>>>
>>>
>>>
>>>> That only debugs the compiler wrapper; use
>>>> gdb --args `gfortran -v file.f90 2>&1 |grep f951`
>>>>
>>>> to debug the real compiler. With "gfc_debug_expr(expr)" you can print the FE
>>>> expression ("p gfc_debug_expr(expr)" in the debugger); a "tree" you can
>>>> print with "p debug_tree(decl)".
>>> Very cool.  Can you give me a tree object that I might want to print
>>> out as an example?
>>>
>>>
>>> I'm running into a bit of a problem now, and I am worried that it may
>>> indicate that I am taking the wrong approach.  Here's an outline of
>>> what I am doing followed by the error I am seeing:
>>>
>>> In parse_derived, I am taking the switch(st) to a function that I have
>>> created called parse_union().  I modeled this after the parse_derived
>>> method but I am getting a seg fault on the modification of
>>> gfc_new_block because the prior call to next_statement sets
>>> gfc_new_block to NULL.
>>>
>>> Am I calling parse_union() at an appropriate time?
>>>
>>> Do I even need to mess with gfc_new_block?  It isn't apparent where it
>>> is used (one of those pesky globals).
>>>
>>> My plan is to nest even further when parsing MAP, by calling eg
>>> parse_map() from within parse_union.  Does that sound kosher?
>>>
>>> What mechanisms should I use for validity checking of statements
>>> within maps/unions... should I roll my own or are there existing
>>> methods that I can extend?

Attachment: union_map.patch
Description: Binary data

Attachment: union_map.for
Description: Binary data