[RFC] unifying gfc_symbol and gfc_component
Janus Weil
janus@gcc.gnu.org
Tue Nov 15 09:49:00 GMT 2016
Hi Paul,
thanks for your comments!
> 'Entities' have a very specific place in the fortran nomenclature and
> are represented by gfc_symbols. Components are not entities. I don't
> say this because of what you have called the base type
well, I am aware that my naming choice was probably not a very good
one (I did not think too hard about it).
Alternatives (possibly not much better) might be: gfc_base_symbol,
gfc_object, ...?
> but rather I
> would need a demonstration that is eliminating duplicated code you
> don't wind up complexifying the code that has to distinguish the two.
In the attachment you can find an extension of the patch, which deals
with one of the cases I mentioned in the beginning, namely
resolve_procedure_interface. As you can see, my approach can remove
quite a bit of code duplication here. However, there are also
problems: First the namespace which I have to pass in separately, and
then the 'result', which is only available in gfc_symbol, but not in
gfc_component. Commenting out the one line that sets the result
apparently leads to a regression in gfortran.dg/proc_decl_25.f90.
For the other case I mentioned (gfc_build_class_symbol) the use of
gfc_entity would not actually remove any code duplication, just make
the code a bit more concise and readable. Unfortunately, it seems that
giving gfc_build_class_symbol an gfc_entity argument would not work
everywhere. In particular there some cases dealing with ASSOCIATE
where gfc_build_class_symbol is used in a different way (probably a
bit perverted from the original intention).
> In general terms, had gfortran been written in C++ from the outset, it
> probably would have been much more concise. However, it would likely
> not have attracted the support of voluntary maintainers over the
> years, who have by and large needed specific features or to fix bugs
> in order to achieve what they need to do. If we start down this road,
> we will wind up with a horrible hybrid that will make gfortran even
> more difficult to maintain. It is already enough that these hard
> working maintainers have had different styles and different approaches
> so that much of the code is distinctly flaky. Just take a look at
> gfc_trans_procedure_call as an example of what I mean.
I partially share your opinion here, but not fully. Of course many
things in gfortran have just grown historically into what they are now
and have seen contributions from many different people with different
styles and ideas, which does not make things easier. But that is part
of my motivation to not just fix bugs one by one, but to think about
how to improve gfortran structurally and to clean it up and make it
more maintainable.
I'm not sure how useful my current proposal is in the end. If other
people can see more use cases for it, it might still be worth to
investigate. But as my examples above show, it will probably not be
straightforward to apply it, even if it such sounds reasonable in
theory. It was just an idea, and if it turns out not to be a useful
one, we can just bury it.
In general, I'm not proposing that we should use as many C++ features
as possible, but I'm sure there are multiple opportunities for
improvement if we selectively use some C++ in places where it makes
sense. I certainly don't want to scare off voluntary maintainers and
contributors, but rather make things easier for them. IMHO everyone
who is fluent enough in C and Fortran to contribute to gfortran will
also be able to deal with some C++ (and I'm not talking about advanced
stuff like template metaprogramming here, but just basics).
Cheers,
Janus
> On 14 November 2016 at 17:35, Fritz Reese <fritzoreese@gmail.com> wrote:
>> Resending as plaintext.
>>
>> On Nov 14, 2016 06:43, "Janus Weil" <janus@gcc.gnu.org> wrote:
>> ...
>>> Now that C++ is officially the implementation language of GCC (and
>>> thus gfortran), it is actually very simple to implement the above
>>> idea: One can just introduce a common base type (I'll call it
>>> gfc_entity for now, but that name is debatable of course), and make
>>> gfc_symbol and gfc_component inherit from that common base type.
>>>
>>> Attached is a patch which does that with just very few changes and
>>> compiles and regtests cleanly. This is just a first draft. One could
>>> certainly share more properties if one accepts slight naming changes
>>> (e.g. locus etc). Also the patch does not make any use of the new
>>> possibilities yet.
>>>
>>> Before I continue with that, I'd like to know what others think about
>>> this idea. Do you think this is useful, or could it cause any harm?
>>>
>>> Cheers,
>>> Janus
>>
>> I for one am generally excited about taking advantage of new features.
>> I think the patch looks good and see no harm done. In fact I think
>> locus should be shared too.
>>
>> ---
>> Fritz Reese
-------------- next part --------------
Index: gcc/fortran/gfortran.h
===================================================================
--- gcc/fortran/gfortran.h (Revision 242395)
+++ gcc/fortran/gfortran.h (Arbeitskopie)
@@ -1033,16 +1033,24 @@ gfc_array_spec;
#define gfc_get_array_spec() XCNEW (gfc_array_spec)
-/* Components of derived types. */
-typedef struct gfc_component
+/* Some entity with a name, typespec, attributes etc.
+ * Base type for gfc_component and gfc_symbol. */
+typedef struct gfc_entity
{
- const char *name;
- gfc_typespec ts;
+ const char *name; /* Primary name, before renaming */
+ gfc_typespec ts; /* Type specification */
- symbol_attribute attr;
- gfc_array_spec *as;
+ symbol_attribute attr; /* Attributes */
+ gfc_array_spec *as; /* Array specification */
tree backend_decl;
+}
+gfc_entity;
+
+
+/* Components of derived types. */
+typedef struct gfc_component : gfc_entity
+{
/* Used to cache a FIELD_DECL matching this same component
but applied to a different backend containing type that was
generated by gfc_nonrestricted_type. */
@@ -1481,15 +1489,11 @@ gfc_typebound_proc;
symtree structures that is balanced by the red-black method-- more
than one symtree node can point to any given symbol. */
-typedef struct gfc_symbol
+typedef struct gfc_symbol : gfc_entity
{
- const char *name; /* Primary name, before renaming */
const char *module; /* Module this symbol came from */
locus declared_at;
- gfc_typespec ts;
- symbol_attribute attr;
-
/* The formal member points to the formal argument list if the
symbol is a function or subroutine name. If the symbol is a
generic name, the generic member points to the list of
@@ -1503,7 +1507,6 @@ gfc_typebound_proc;
struct gfc_namespace *f2k_derived;
struct gfc_expr *value; /* Parameter/Initializer value */
- gfc_array_spec *as;
struct gfc_symbol *result; /* function result symbol */
gfc_component *components; /* Derived type components */
@@ -1554,8 +1557,6 @@ gfc_typebound_proc;
int refs;
struct gfc_namespace *ns; /* namespace containing this symbol */
- tree backend_decl;
-
/* Identity of the intrinsic module the symbol comes from, or
INTMOD_NONE if it's not imported from a intrinsic module. */
intmod_id from_intmod;
Index: gcc/fortran/resolve.c
===================================================================
--- gcc/fortran/resolve.c (Revision 242395)
+++ gcc/fortran/resolve.c (Arbeitskopie)
@@ -185,68 +185,55 @@ check_proc_interface (gfc_symbol *ifc, locus *wher
static void resolve_symbol (gfc_symbol *sym);
-/* Resolve the interface for a PROCEDURE declaration or procedure pointer. */
+/* Copy typespec, attributes, etc from an interface symbol to entity e. */
-static bool
-resolve_procedure_interface (gfc_symbol *sym)
+static bool copy_interface (gfc_symbol *ifc, gfc_entity *e, gfc_namespace *ns)
{
- gfc_symbol *ifc = sym->ts.interface;
-
- if (!ifc)
- return true;
-
- if (ifc == sym)
- {
- gfc_error ("PROCEDURE %qs at %L may not be used as its own interface",
- sym->name, &sym->declared_at);
- return false;
- }
- if (!check_proc_interface (ifc, &sym->declared_at))
- return false;
-
if (ifc->attr.if_source || ifc->attr.intrinsic)
{
- /* Resolve interface and copy attributes. */
- resolve_symbol (ifc);
+ /* Resolve interface. */
+ if (ifc->formal && !ifc->formal_ns)
+ resolve_symbol (ifc);
if (ifc->attr.intrinsic)
gfc_resolve_intrinsic (ifc, &ifc->declared_at);
+ /* Copy typespec and attributes. */
if (ifc->result)
{
- sym->ts = ifc->result->ts;
- sym->attr.allocatable = ifc->result->attr.allocatable;
- sym->attr.pointer = ifc->result->attr.pointer;
- sym->attr.dimension = ifc->result->attr.dimension;
- sym->attr.class_ok = ifc->result->attr.class_ok;
- sym->as = gfc_copy_array_spec (ifc->result->as);
- sym->result = sym;
+ e->ts = ifc->result->ts;
+ e->attr.allocatable = ifc->result->attr.allocatable;
+ e->attr.pointer = ifc->result->attr.pointer;
+ e->attr.dimension = ifc->result->attr.dimension;
+ e->attr.class_ok = ifc->result->attr.class_ok;
+ e->as = gfc_copy_array_spec (ifc->result->as);
+ /*e->result = e;*/
}
else
{
- sym->ts = ifc->ts;
- sym->attr.allocatable = ifc->attr.allocatable;
- sym->attr.pointer = ifc->attr.pointer;
- sym->attr.dimension = ifc->attr.dimension;
- sym->attr.class_ok = ifc->attr.class_ok;
- sym->as = gfc_copy_array_spec (ifc->as);
+ e->ts = ifc->ts;
+ e->attr.allocatable = ifc->attr.allocatable;
+ e->attr.pointer = ifc->attr.pointer;
+ e->attr.dimension = ifc->attr.dimension;
+ e->attr.class_ok = ifc->attr.class_ok;
+ e->as = gfc_copy_array_spec (ifc->as);
}
- sym->ts.interface = ifc;
- sym->attr.function = ifc->attr.function;
- sym->attr.subroutine = ifc->attr.subroutine;
+ e->ts.interface = ifc;
+ e->attr.function = ifc->attr.function;
+ e->attr.subroutine = ifc->attr.subroutine;
- sym->attr.pure = ifc->attr.pure;
- sym->attr.elemental = ifc->attr.elemental;
- sym->attr.contiguous = ifc->attr.contiguous;
- sym->attr.recursive = ifc->attr.recursive;
- sym->attr.always_explicit = ifc->attr.always_explicit;
- sym->attr.ext_attr |= ifc->attr.ext_attr;
- sym->attr.is_bind_c = ifc->attr.is_bind_c;
+ e->attr.pure = ifc->attr.pure;
+ e->attr.elemental = ifc->attr.elemental;
+ e->attr.contiguous = ifc->attr.contiguous;
+ e->attr.recursive = ifc->attr.recursive;
+ e->attr.always_explicit = ifc->attr.always_explicit;
+ e->attr.ext_attr |= ifc->attr.ext_attr;
+ e->attr.is_bind_c = ifc->attr.is_bind_c;
/* Copy char length. */
if (ifc->ts.type == BT_CHARACTER && ifc->ts.u.cl)
{
- sym->ts.u.cl = gfc_new_charlen (sym->ns, ifc->ts.u.cl);
- if (sym->ts.u.cl->length && !sym->ts.u.cl->resolved
- && !gfc_resolve_expr (sym->ts.u.cl->length))
+ e->ts.u.cl = gfc_new_charlen (ns, ifc->ts.u.cl);
+ if (e->ts.u.cl->length && !e->ts.u.cl->resolved
+ && !gfc_resolve_expr (e->ts.u.cl->length))
return false;
}
}
@@ -255,6 +242,29 @@ static void resolve_symbol (gfc_symbol *sym);
}
+/* Resolve the interface for a PROCEDURE declaration or procedure pointer. */
+
+static bool
+resolve_procedure_interface (gfc_symbol *sym)
+{
+ gfc_symbol *ifc = sym->ts.interface;
+
+ if (!ifc)
+ return true;
+
+ if (ifc == sym)
+ {
+ gfc_error ("PROCEDURE %qs at %L may not be used as its own interface",
+ sym->name, &sym->declared_at);
+ return false;
+ }
+ if (!check_proc_interface (ifc, &sym->declared_at))
+ return false;
+
+ return copy_interface (ifc, sym, sym->ns);
+}
+
+
/* Resolve types of formal argument lists. These have to be done early so that
the formal argument lists of module procedures can be copied to the
containing module before the individual procedures are resolved
@@ -13302,54 +13312,11 @@ resolve_component (gfc_component *c, gfc_symbol *s
return false;
}
- if (ifc->attr.if_source || ifc->attr.intrinsic)
- {
- /* Resolve interface and copy attributes. */
- if (ifc->formal && !ifc->formal_ns)
- resolve_symbol (ifc);
- if (ifc->attr.intrinsic)
- gfc_resolve_intrinsic (ifc, &ifc->declared_at);
-
- if (ifc->result)
- {
- c->ts = ifc->result->ts;
- c->attr.allocatable = ifc->result->attr.allocatable;
- c->attr.pointer = ifc->result->attr.pointer;
- c->attr.dimension = ifc->result->attr.dimension;
- c->as = gfc_copy_array_spec (ifc->result->as);
- c->attr.class_ok = ifc->result->attr.class_ok;
- }
- else
- {
- c->ts = ifc->ts;
- c->attr.allocatable = ifc->attr.allocatable;
- c->attr.pointer = ifc->attr.pointer;
- c->attr.dimension = ifc->attr.dimension;
- c->as = gfc_copy_array_spec (ifc->as);
- c->attr.class_ok = ifc->attr.class_ok;
- }
- c->ts.interface = ifc;
- c->attr.function = ifc->attr.function;
- c->attr.subroutine = ifc->attr.subroutine;
-
- c->attr.pure = ifc->attr.pure;
- c->attr.elemental = ifc->attr.elemental;
- c->attr.recursive = ifc->attr.recursive;
- c->attr.always_explicit = ifc->attr.always_explicit;
- c->attr.ext_attr |= ifc->attr.ext_attr;
- /* Copy char length. */
- if (ifc->ts.type == BT_CHARACTER && ifc->ts.u.cl)
- {
- gfc_charlen *cl = gfc_new_charlen (sym->ns, ifc->ts.u.cl);
- if (cl->length && !cl->resolved
- && !gfc_resolve_expr (cl->length))
- {
- c->tb->error = 1;
- return false;
- }
- c->ts.u.cl = cl;
- }
- }
+ if (!copy_interface (ifc, c, sym->ns))
+ {
+ c->tb->error = 1;
+ return false;
+ }
}
else if (c->attr.proc_pointer && c->ts.type == BT_UNKNOWN)
{
More information about the Fortran
mailing list