Inheritance of gfc_symbol / gfc_component

[Tobias Schlüter]

Hi Janus,

another former gfortraner joins the conversation (I do program mostly 
C++ nowadays, though I rarely design large pieces of code).  As is, I 
don't like this patch, because it doesn't simplify anything, just adds a 
layer that has no intrinsic meaning.  You now have to look for the 
structure members in two places without any subsequent simplification in 
the code.  I think it shouldn't be installed without accompanying 
simplifications say in the functions you pointed to.  Generally, it is 
my opinion that working code should only be changed if an improvement of 
whatever kind is demonstrated.

A place where C++ inheritance is a trivial improvement is the red-black 
tree used for storing various objects (gfc_symtree, gfc_gsymbol, 
gfc_st_label, I think).  This is currently implemented with macro-based 
inheritance.  It is trivial to replace the macro with C++ inheritance, 
but if one touches this code, it might make more sense to do a real job 
instead and to convert the symbol-keeping code to the compiler's 
hash-table implementation, which as a benefit should make the compiler 
faster by get rid of lots of string comparisons.

As a more involved project, and something where one could really gain by 
going to C++ would be to replace the large union in, say, gfc_expr, with 
inheritance.  A second step would then be to replace the resolvers etc. 
for the various types of expressions to member functions of the then-new 
derived classes gfc_expr_operator, gfc_expr_function etc., but even that 
may not turn out to be a net gain in terms of code clarity.

Cheers,
- Tobi

ps looking at gfortran.h another trivial project where C++ simplifies 
the code would be to replace
      typedef struct gfc_linebuf { ... } gfc_linebuf;
with
      struct gfc_linebuf { ... };
and similar.

On 2012-08-16 13:00, Janus Weil wrote:
> Hi Daniel,
>
>> I know I've been inactive a very long time (because of lots of other
>> things I had/have to do), but reading this by chance, I'm still going
>> to comment.
>
> nice to hear from you after such a long time, and thanks for your comments.
>
> Btw, in case you're interested to return to gfortran: The
> implementation of FINAL is becoming acute again right now (which, I
> believe, used to be an old love of yours). See Tobias' recent patch:
> http://gcc.gnu.org/ml/fortran/2012-08/msg00057.html
>
>
>>> One of the first things that comes to my mind, when thinking in
>>> this direction, is that the structs 'gfc_component' and
>>> 'gfc_symbol' have a lot in common (name, ts, attr, as, just to
>>> name a few), so that one could think about having gfc_symbol
>>> inherit from gfc_component (in Fortran this would be called "type
>>> extension", but I think the correct C++ term is "inheritance").
>>> There are a couple of routines in the front end which basically
>>> act the same way on components and symbols and which could be
>>> unified with such an approach (by receiving a pointer, which can
>>> either point to a gfc_symbol or a gfc_component). Examples:
>>> gfc_copy_formal_args, gfc_expr_replace_symbols,
>>> resolve_procedure_interface.
>>
>> I don't know what others here think about using C++ (I remember that
>> some gfortraners weren't fluent in C++ when the C++ branch was first
>> discussed), but I personally use C++ over C in my projects and believe
>> this could really help.
>
> There has been a considerable amount of discussion regarding GCC's
> transition to C++, and I think there was some sort of consensus to
> stick to a basic subset of C++ and not use the more 'obscure' features
> (cf. also http://gcc.gnu.org/codingconventions.html#Cxx_Conventions).
>
> Regarding things like inheritance: This sort of concept is even known
> in Fortran these days, so it should not be too exotic for anyone
> working on the Fortran front end, I guess ...
>
>
>>> Attached you find a very short and simple patch, which implements
>>> the trivial part of this inheritance approach: It makes gfc_symbol
>>> inherit from gfc_component, and removes those 7 members from
>>> gfc_symbol, which are literally equivalent in both structs. (The
>>> patch compiles cleanly, but is not regtested.)
>>>
>>> In addition to these 7 'common' members, there are three more
>>> members which are more-or-less equivalent, but named differently:
>>> * 'loc' vs 'declared at' * 'initializer' vs 'value' * 'next' vs
>>> 'components' These could be replaced by purely mechanical efforts.
>>>
>>> This leaves only two members of gfc_component, which are not
>>> shared by gfc_symbol: * norestrict_decl * tb
>>>
>>> In the current simple form of the patch, these would slightly blow
>>> up gfc_symbol (being unused there). An alternative could be to set
>>> up a base struct, from which both gfc_component and gfc_symbol
>>> would inherit.
>>
>> I would be in favour of the common base class approach, because it is
>> much cleaner.  It not only doesn't introduce the useless two members
>> to gfc_symbol (which might later confuse people trying to find out
>> what they mean!), but if you did the conversion as you suggest, this
>> would also mean that all gfc_symbol's are "considered to be"
>> gfc_components as well (and accepted by routines operating on
>> gfc_components, even if they are really meant only for components) --
>> which is not what it should be in my opinion, since gfc_symbol and
>> gfc_component are similar but distinct concepts.
>>
>> Using a common base class keeps gfc_symbol and gfc_component as
>> different where they should be, but allows to share members and also
>> to combine routines that are really meant to work on both -- those
>> would then have to be changed to accept a pointer to the base class,
>> as an "explicit action" confirming that they really should accept both
>> classes.
>
> Attached is a new version of the patch, which implements a
> "gfc_base_symbol" as an ancestor for gfc_component and gfc_symbol. For
> now it only has the seven basic members, which have equal names.
>
> On top of this, I think also loc/declared_at and initializer/value
> should be put into the base type (in a mechanical follow-up patch),
> since they're really equivalent.
>
> Does anyone want to OK this patch already, or should I first implement
> some of the applications of the new struct?
>
> Cheers,
> Janus