Re: Inheritance of gfc_symbol / gfc_component

Re: Inheritance of gfc_symbol / gfc_component

[Tobias Schlüter]

Following up on myself:

On 2012-08-16 14:59, Tobias Schlüter wrote:
> 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,

So I have a patch for this which passes the testsuite, but I'm not sure 
if it's valid C++ because of one issue: in io.c we have at file scope

gfc_st_label
format_asterisk = {0, NULL, NULL, -1, ST_LABEL_FORMAT, ST_LABEL_FORMAT, 
NULL,
                   0, {NULL, NULL}};

which doesn't work if I redefine gfc_st_label to inherit from a gfc_bbt 
which contains the balanced binary tree info, so I have
struct gfc_bbt {
   int priority;
   gfc_bbt *left;
   gfc_bbt *right;
};
and
struct gfc_st_label : public gfc_bbt
{
   int value;
  ...
}

Previously a macro made the first three elements of gfc_st_label what 
are now the elements of gfc_bbt.

The problem is that the initialization of format_asterisk is not allowed 
in the C++ standard (according to the error message, C++11 actually 
allows it).

I thought I could work around this problem without introducing a 
constructor by:
1) using 0 instead of -1 as value for this fake label (which is also
    not a valid value for a label, so it can't collide
2) setting ST_LABEL_FORMAT = 0
and then
3) not initializing at all, assuming that as for a C struct
    format_asterisk would end up in .bss and thus be zero initialized.

When I was investigating whether this theory is sound, I understood that 
anything involving inheritance is not POD (plain old data), and 
therefore I can't assume zeroing to be guaranteed.  Is that correct?  If 
that is correct, I will submit the patch.  Otherwise, I would be 
interested if there's an equally simple way of initializing 
format_asterisk once it uses inheritance.  Aternatively, I don't see 
what speaks against allowing C++11 in frontends (well, maybe the famous 
binary incompatibility with C++03 ...)

That said, I had to introduce lots of type-casts in the patch, because I 
didn't want to work with templates, while using typesafe interfaces 
wherever possible, so the functionally equivalent code actually became 
insignificantly larger with C++ replacing macro-based inheritance.

Cheers,
- Tobi

Re: Inheritance of gfc_symbol / gfc_component

[Mikael Morin]

On 17/08/2012 23:32, Tobias Schlüter wrote:
> 
> Following up on myself:
> 
> On 2012-08-16 14:59, Tobias Schlüter wrote:
>> 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,
> 
> So I have a patch for this which passes the testsuite, but I'm not sure
> if it's valid C++ 
[...]
> The problem is that the initialization of format_asterisk is not allowed
> in the C++ standard
> 
So it's not valid ;-)

> I thought I could work around this problem without introducing a
> constructor by:
> 1) using 0 instead of -1 as value for this fake label (which is also
>    not a valid value for a label, so it can't collide
> 2) setting ST_LABEL_FORMAT = 0
> and then
> 3) not initializing at all, assuming that as for a C struct
>    format_asterisk would end up in .bss and thus be zero initialized.
> 
I would use a constructor.

> That said, I had to introduce lots of type-casts in the patch, because I
> didn't want to work with templates,
I don't like templates as it looks ugly in the code, but I think that in
this case it is the proper way to represent a bbt: the bbt is a way to
access the data, not part of the data itself.  By the way, I'm not sure
the bbt has any value over the STL's map.

> while using typesafe interfaces
> wherever possible, so the functionally equivalent code actually became
> insignificantly larger with C++ replacing macro-based inheritance.
>
The bbt is a well separated part of the frontend with a small set of
well defined "methods" working on it.  As a result of this, it is easy
to change it's internal implementation or the way it is accessed; as
long as the interfaces remain the same, any change to it would be
basically "insignificant".  To put the story short, I don't think the
bbt is that important (which doesn't mean I'm opposed to any change).

On the other hand, If we go the inheritance way, I think that we could
save some code by moving manual bbt walking functions from symbol.c and
module.c into member functions (i.e. start doing some real cleanup).

Could you post the not-yet-finished patch?

Mikael

Re: Inheritance of gfc_symbol / gfc_component

[Tobias Schlüter]

[-- Attachment #1: Type: text/plain, Size: 3690 bytes --]


Dear Mikael,

On 2012-08-18 13:41, Mikael Morin wrote:
> On 17/08/2012 23:32, Tobias Schlüter wrote:
>> The problem is that the initialization of format_asterisk is not allowed
>> in the C++ standard
>>
> So it's not valid ;-)

Sure, that's why I tried to work around it.  My question was: is the 
workaround valid?

>> I thought I could work around this problem without introducing a
>> constructor by:
>> 1) using 0 instead of -1 as value for this fake label (which is also
>>     not a valid value for a label, so it can't collide
>> 2) setting ST_LABEL_FORMAT = 0
>> and then
>> 3) not initializing at all, assuming that as for a C struct
>>     format_asterisk would end up in .bss and thus be zero initialized.
>>
> I would use a constructor.

Which would defeat the purpose of this patch: simpler code (no macro 
magic) + more typesafety.  A constructor which is needed for a single 
case is no simplification IMO.

>> That said, I had to introduce lots of type-casts in the patch, because I
>> didn't want to work with templates,
> I don't like templates as it looks ugly in the code, but I think that in
> this case it is the proper way to represent a bbt: the bbt is a way to
> access the data, not part of the data itself.  By the way, I'm not sure
> the bbt has any value over the STL's map.

No templates are allowed by the coding standard.  The benefit over the 
STL's map or a hashtable (which is better suited) is that we can 
traverse a tree in defined order.  We use this property to write 
reproducible module files.  But sure, a bigger plan would be to move to 
the compiler's hashtable code, if one introduced ordering in the places 
where we care about it.

>> while using typesafe interfaces
>> wherever possible, so the functionally equivalent code actually became
>> insignificantly larger with C++ replacing macro-based inheritance.
>>
> The bbt is a well separated part of the frontend with a small set of
> well defined "methods" working on it.  As a result of this, it is easy
> to change it's internal implementation or the way it is accessed; as
> long as the interfaces remain the same, any change to it would be
> basically "insignificant".  To put the story short, I don't think the
> bbt is that important (which doesn't mean I'm opposed to any change).

Sure, I had hoped it would be a non-invasive change, which due to the 
constructor issue, it didn't turn out to be.  Note that I need all of 
these typecasts because I decided to make the interfaces of the various 
recursive functions use the specific types in their prototypes, I 
thought this was clearer than the other possibility of using gfc_bbt in 
the prototypes and then casting to the specific type when the pointer's 
properties are accessed.  That way the type wouldn't be exposed in the 
function prototype.

> On the other hand, If we go the inheritance way, I think that we could
> save some code by moving manual bbt walking functions from symbol.c and
> module.c into member functions (i.e. start doing some real cleanup).
>
> Could you post the not-yet-finished patch?

Please find it attached.  Note that two void* remain: 
gfc_{insert|delete}_bbt still need it, and I don't think there's a way 
around this without templates and without significantly changing the 
code.  As I said before, I think a change needs to have a benefit.  I'm 
disappointed by this outcome, but think this patch fails this 
requirement mainly because of the format_asterisk issue.

(In the process I added const to the comparator function, which added a 
few more diffs I wouldn't have needed otherwise.)

Cheers,
- Tobi


[-- Attachment #2: patch_bbt.diff.txt --]
[-- Type: text/plain, Size: 25873 bytes --]

2012-08-18  Tobias Schlüter  <tobi@gcc.gnu.org>

	* gfortran.h (gfc_sl_type): Set ST_LABEL_FORMAT to 0.
	(BBT_HEADER): Remove.  Replace with ...
	(gfc_bbt): ... new type.
	(gfc_st_label): Inherit from gfc_bbt instead of macro magic.
	Use C++ declaration syntax.
	(gfc_symtree): Likewise.
	(gfc_symbol): Likewise.
	(compare_fn): Replace void* with typed pointer.
	(gfc_insert_bbt): Likewise.
	(gfc_delete_bbt): Likewise.
	* bbt.c (gfc_bbt): Remove.
	(gfc_insert_bbt): Use typed pointer.
	(gfc_delete_bbt): Likewise.
	* class.c (add_procs_to_declared_vtabl): Add typecasts.
	* dump-parse-tree.c (traverse_uop): Same.
	* interface.c (find_symtree0): Same.
	* io.c (format_asterisk): Remove initializer.
	* module.c (pointer_info): Inherit from gfc_bbt.  Use C++
	declaration syntax.
	(free_pi_tree): Add typecasts.
	(compare_pointers): Change arguments to const gfc_bbt*, adapt casts.
	(compare_integers): Same.
	(find_pointer): Add typecasts.
	(get_integer): Same.
	(fp2): Same.
	(true_name): Inherit from gfc_bbt.  Use C++ declaration syntax.
	(compare_true_names): Change arguments to const gfc_bbt*, adapt
	casts.
	(build_tnt): Add typecasts.
	(free_true_name): Same.
	(find_symtree_for_symbol): Same.
	(find_symbol): Same.
	(load_needed): Same.
	(read_cleanup): Same.
	(written_common): Inherit from gfc_bbt.
	(compare_written_commons): Change arguments to const gfc_bbt*,
	adapt casts.
	(free_written_common): No 'struct' in prototype.  Add typecasts.
	(write_common_0): Add typecasts.
	(write_symbol0): Same.
	(write_symbol1): Same.
	(write_generic): Same.
	* parse.c (clean_up_modules): Same.
	* resolve.c (resolve_common_blocks): Same.
	(ensure_not_abstract_walker): Same.
	(warn_unused_fortran_label): Same.
	(gfc_resolve_uops): Same.
	* symbol.c (switch_types): Same.
	(compare_st_labels): Change arguments to const gfc_bbt*, adapt
	casts.
	(free_st_labels): Add typecasts.
	(gfc_get_st_label): Same.
	(compare_symtree): Same.
	(gfc_find_symtree): Same.
	(free_tb_tree): Same.
	(free_common_tree): Same.
	(free_uop_tree): Same.
	(free_sym_tree): Same.
	(count_st_nodes): Same.
	(fill_st_vector): Same.
	(gfc_find_gsymbol): Same.
	(gsym_compare): Change arguments to const gfc_bbt*, adapt casts.


diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index 7c4c0a4..6e03548 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -149,7 +149,7 @@ ar_type;
    nor CONTINUE; otherwise it is identical to ST_LABEL_TARGET.  */
 typedef enum
 { ST_LABEL_UNKNOWN = 1, ST_LABEL_TARGET, ST_LABEL_DO_TARGET,
-  ST_LABEL_BAD_TARGET, ST_LABEL_FORMAT
+  ST_LABEL_BAD_TARGET, ST_LABEL_FORMAT = 0
 }
 gfc_sl_type;
 
@@ -595,7 +595,11 @@ gfc_reverse;
 /************************* Structures *****************************/
 
 /* Used for keeping things in balanced binary trees.  */
-#define BBT_HEADER(self) int priority; struct self *left, *right
+struct gfc_bbt {
+  int priority;
+  gfc_bbt *left;
+  gfc_bbt *right;
+};
 
 #define NAMED_INTCST(a,b,c,d) a,
 #define NAMED_KINDARRAY(a,b,c,d) a,
@@ -1075,10 +1079,8 @@ gfc_omp_clauses;
 /* The gfc_st_label structure is a BBT attached to a namespace that
    records the usage of statement labels within that space.  */
 
-typedef struct gfc_st_label
+struct gfc_st_label : public gfc_bbt
 {
-  BBT_HEADER(gfc_st_label);
-
   int value;
 
   gfc_sl_type defined, referenced;
@@ -1088,8 +1090,7 @@ typedef struct gfc_st_label
   tree backend_decl;
 
   locus where;
-}
-gfc_st_label;
+};
 
 
 /* gfc_interface()-- Interfaces are lists of symbols strung together.  */
@@ -1325,9 +1326,8 @@ gfc_use_list;
    several symtrees pointing to the same symbol node via USE
    statements.  */
 
-typedef struct gfc_symtree
+struct gfc_symtree : public gfc_bbt
 {
-  BBT_HEADER (gfc_symtree);
   const char *name;
   int ambiguous;
   union
@@ -1338,8 +1338,7 @@ typedef struct gfc_symtree
     gfc_typebound_proc *tb;
   }
   n;
-}
-gfc_symtree;
+};
 
 /* A linked list of derived types in the namespace.  */
 typedef struct gfc_dt_list
@@ -1464,10 +1463,8 @@ enum gfc_symbol_type
   GSYM_MODULE, GSYM_COMMON, GSYM_BLOCK_DATA
 };
 
-typedef struct gfc_gsymbol
+struct gfc_gsymbol : public gfc_bbt
 {
-  BBT_HEADER(gfc_gsymbol);
-
   const char *name;
   const char *sym_name;
   const char *mod_name;
@@ -1477,8 +1474,7 @@ typedef struct gfc_gsymbol
   int defined, used;
   locus where;
   gfc_namespace *ns;
-}
-gfc_gsymbol;
+};
 
 extern gfc_gsymbol *gfc_gsym_root;
 
@@ -2911,9 +2907,9 @@ void gfc_init_coarray_decl (bool);
 bool gfc_inline_intrinsic_function_p (gfc_expr *);
 
 /* bbt.c */
-typedef int (*compare_fn) (void *, void *);
-void gfc_insert_bbt (void *, void *, compare_fn);
-void gfc_delete_bbt (void *, void *, compare_fn);
+typedef int (*compare_fn) (const gfc_bbt *, const gfc_bbt *);
+void gfc_insert_bbt (void *, gfc_bbt *, compare_fn);
+void gfc_delete_bbt (void *, gfc_bbt *, compare_fn);
 
 /* dump-parse-tree.c */
 void gfc_dump_parse_tree (gfc_namespace *, FILE *);
diff --git a/gcc/fortran/bbt.c b/gcc/fortran/bbt.c
index 000f04b..7064fa3 100644
--- a/gcc/fortran/bbt.c
+++ b/gcc/fortran/bbt.c
@@ -41,11 +41,6 @@ along with GCC; see the file COPYING3.  If not see
 #include "coretypes.h"
 #include "gfortran.h"
 
-typedef struct gfc_treap
-{
-  BBT_HEADER (gfc_treap);
-}
-gfc_bbt;
 
 /* Simple linear congruential pseudorandom number generator.  The
    period of this generator is 44071, which is plenty for our
@@ -128,14 +123,11 @@ insert (gfc_bbt *new_bbt, gfc_bbt *t, compare_fn compare)
    already exists.  */
 
 void
-gfc_insert_bbt (void *root, void *new_node, compare_fn compare)
+gfc_insert_bbt (void *root, gfc_bbt *new_node, compare_fn compare)
 {
-  gfc_bbt **r, *n;
-
-  r = (gfc_bbt **) root;
-  n = (gfc_bbt *) new_node;
-  n->priority = pseudo_random ();
-  *r = insert (n, *r, compare);
+  gfc_bbt **r = (gfc_bbt **)root;
+  new_node->priority = pseudo_random ();
+  *r = insert (new_node, *r, compare);
 }
 
 static gfc_bbt *
@@ -190,10 +182,8 @@ delete_treap (gfc_bbt *old, gfc_bbt *t, compare_fn compare)
 
 
 void
-gfc_delete_bbt (void *root, void *old, compare_fn compare)
+gfc_delete_bbt (void *root, gfc_bbt *old, compare_fn compare)
 {
-  gfc_bbt **t;
-
-  t = (gfc_bbt **) root;
-  *t = delete_treap ((gfc_bbt *) old, *t, compare);
+  gfc_bbt **r = (gfc_bbt **)root;
+  *r = delete_treap (old, *r, compare);
 }
diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c
index 21a91ba..35adedf 100644
--- a/gcc/fortran/class.c
+++ b/gcc/fortran/class.c
@@ -658,10 +658,10 @@ add_procs_to_declared_vtab1 (gfc_symtree *st, gfc_symbol *vtype)
     return;
 
   if (st->left)
-    add_procs_to_declared_vtab1 (st->left, vtype);
+    add_procs_to_declared_vtab1 ((gfc_symtree *)st->left, vtype);
 
   if (st->right)
-    add_procs_to_declared_vtab1 (st->right, vtype);
+    add_procs_to_declared_vtab1 ((gfc_symtree *)st->right, vtype);
 
   if (st->n.tb && !st->n.tb->error 
       && !st->n.tb->is_generic && st->n.tb->u.specific)
diff --git a/gcc/fortran/dump-parse-tree.c b/gcc/fortran/dump-parse-tree.c
index cb8fab4..b6da3d0 100644
--- a/gcc/fortran/dump-parse-tree.c
+++ b/gcc/fortran/dump-parse-tree.c
@@ -938,8 +938,8 @@ traverse_uop (gfc_symtree *st, void (*func) (gfc_user_op *))
 
   (*func) (st->n.uop);
 
-  traverse_uop (st->left, func);
-  traverse_uop (st->right, func);
+  traverse_uop ((gfc_symtree *)st->left, func);
+  traverse_uop ((gfc_symtree *)st->right, func);
 }
 
 
diff --git a/gcc/fortran/interface.c b/gcc/fortran/interface.c
index 482c294..b18bcd5 100644
--- a/gcc/fortran/interface.c
+++ b/gcc/fortran/interface.c
@@ -3321,9 +3321,9 @@ find_symtree0 (gfc_symtree *root, gfc_symbol *sym)
 
   st = NULL;
   if (root->left)
-    st = find_symtree0 (root->left, sym);
+    st = find_symtree0 ((gfc_symtree *)root->left, sym);
   if (root->right && ! st)
-    st = find_symtree0 (root->right, sym);
+    st = find_symtree0 ((gfc_symtree *)root->right, sym);
   return st;
 }
 
diff --git a/gcc/fortran/io.c b/gcc/fortran/io.c
index 428799c..298efb2 100644
--- a/gcc/fortran/io.c
+++ b/gcc/fortran/io.c
@@ -29,8 +29,8 @@ along with GCC; see the file COPYING3.  If not see
 #include "parse.h"
 
 gfc_st_label
-format_asterisk = {0, NULL, NULL, -1, ST_LABEL_FORMAT, ST_LABEL_FORMAT, NULL,
-		   0, {NULL, NULL}};
+format_asterisk;
 
 typedef struct
 {
diff --git a/gcc/fortran/module.c b/gcc/fortran/module.c
index a4ff199..4de8e61 100644
--- a/gcc/fortran/module.c
+++ b/gcc/fortran/module.c
@@ -138,9 +138,8 @@ enum gfc_wsym_state
   WRITTEN
 };
 
-typedef struct pointer_info
+struct pointer_info : public gfc_bbt
 {
-  BBT_HEADER (pointer_info);
   int integer;
   pointer_t type;
 
@@ -174,8 +173,7 @@ typedef struct pointer_info
   }
   u;
 
-}
-pointer_info;
+};
 
 #define gfc_get_pointer_info() XCNEW (pointer_info)
 
@@ -225,8 +223,8 @@ free_pi_tree (pointer_info *p)
   if (p->fixup != NULL)
     gfc_internal_error ("free_pi_tree(): Unresolved fixup");
 
-  free_pi_tree (p->left);
-  free_pi_tree (p->right);
+  free_pi_tree ((pointer_info *)p->left);
+  free_pi_tree ((pointer_info *)p->right);
 
   if (iomode == IO_INPUT)
     {
@@ -243,12 +241,12 @@ free_pi_tree (pointer_info *p)
    module.  */
 
 static int
-compare_pointers (void *_sn1, void *_sn2)
+compare_pointers (const gfc_bbt *_sn1, const gfc_bbt *_sn2)
 {
-  pointer_info *sn1, *sn2;
+  const pointer_info *sn1, *sn2;
 
-  sn1 = (pointer_info *) _sn1;
-  sn2 = (pointer_info *) _sn2;
+  sn1 = (const pointer_info *) _sn1;
+  sn2 = (const pointer_info *) _sn2;
 
   if (sn1->u.pointer < sn2->u.pointer)
     return -1;
@@ -263,12 +261,12 @@ compare_pointers (void *_sn1, void *_sn2)
    module.  */
 
 static int
-compare_integers (void *_sn1, void *_sn2)
+compare_integers (const gfc_bbt *_sn1, const gfc_bbt *_sn2)
 {
-  pointer_info *sn1, *sn2;
+  const pointer_info *sn1, *sn2;
 
-  sn1 = (pointer_info *) _sn1;
-  sn2 = (pointer_info *) _sn2;
+  sn1 = (const pointer_info *) _sn1;
+  sn2 = (const pointer_info *) _sn2;
 
   if (sn1->integer < sn2->integer)
     return -1;
@@ -323,7 +321,7 @@ find_pointer (void *gp)
     {
       if (p->u.pointer == gp)
 	break;
-      p = (gp < p->u.pointer) ? p->left : p->right;
+      p =  (pointer_info *)((gp < p->u.pointer) ?p->left : p->right);
     }
 
   return p;
@@ -372,7 +370,7 @@ get_integer (int integer)
       if (c == 0)
 	break;
 
-      p = (c < 0) ? p->left : p->right;
+      p = (pointer_info *)((c < 0) ? p->left : p->right);
     }
 
   if (p != NULL)
@@ -401,11 +399,11 @@ fp2 (pointer_info *p, const void *target)
   if (p->u.pointer == target)
     return p;
 
-  q = fp2 (p->left, target);
+  q = fp2 ((pointer_info *)p->left, target);
   if (q != NULL)
     return q;
 
-  return fp2 (p->right, target);
+  return fp2 ((pointer_info *)p->right, target);
 }
 
 
@@ -843,13 +841,11 @@ find_use_operator (gfc_intrinsic_op op)
    as symbols are read.  The tree is searched as we load new symbols
    to see if it already exists someplace in the namespace.  */
 
-typedef struct true_name
+struct true_name : public gfc_bbt
 {
-  BBT_HEADER (true_name);
   const char *name;
   gfc_symbol *sym;
-}
-true_name;
+};
 
 static true_name *true_name_root;
 
@@ -857,13 +853,13 @@ static true_name *true_name_root;
 /* Compare two true_name structures.  */
 
 static int
-compare_true_names (void *_t1, void *_t2)
+compare_true_names (const gfc_bbt *_t1, const gfc_bbt *_t2)
 {
-  true_name *t1, *t2;
+  const true_name *t1, *t2;
   int c;
 
-  t1 = (true_name *) _t1;
-  t2 = (true_name *) _t2;
+  t1 = (const true_name *) _t1;
+  t2 = (const true_name *) _t2;
 
   c = ((t1->sym->module > t2->sym->module)
        - (t1->sym->module < t2->sym->module));
@@ -894,11 +890,11 @@ find_true_name (const char *name, const char *module)
   p = true_name_root;
   while (p != NULL)
     {
-      c = compare_true_names ((void *) (&t), (void *) p);
+      c = compare_true_names (&t, p);
       if (c == 0)
 	return p->sym;
 
-      p = (c < 0) ? p->left : p->right;
+      p = (c < 0) ? (true_name *)p->left : (true_name *)p->right;
     }
 
   return NULL;
@@ -933,8 +929,8 @@ build_tnt (gfc_symtree *st)
   if (st == NULL)
     return;
 
-  build_tnt (st->left);
-  build_tnt (st->right);
+  build_tnt ((gfc_symtree *)st->left);
+  build_tnt ((gfc_symtree *)st->right);
 
   if (st->n.sym->attr.flavor == FL_DERIVED)
     name = dt_upper_string (st->n.sym->name);
@@ -965,8 +961,8 @@ free_true_name (true_name *t)
 {
   if (t == NULL)
     return;
-  free_true_name (t->left);
-  free_true_name (t->right);
+  free_true_name ((true_name *)t->left);
+  free_true_name ((true_name *)t->right);
 
   free (t);
 }
@@ -3878,10 +3874,10 @@ find_symtree_for_symbol (gfc_symtree *st, gfc_symbol *sym)
   if (st == NULL)
     return s;
 
-  s = find_symtree_for_symbol (st->right, sym);
+  s = find_symtree_for_symbol ((gfc_symtree *)st->right, sym);
   if (s != NULL)
     return s;
-  s = find_symtree_for_symbol (st->left, sym);
+  s = find_symtree_for_symbol ((gfc_symtree *)st->left, sym);
   if (s != NULL)
     return s;
 
@@ -3923,10 +3919,10 @@ find_symbol (gfc_symtree *st, const char *name,
 	return st;
     }
 
-  retval = find_symbol (st->left, name, module, generic);
+  retval = find_symbol ((gfc_symtree *)st->left, name, module, generic);
 
   if (retval == NULL)
-    retval = find_symbol (st->right, name, module, generic);
+    retval = find_symbol ((gfc_symtree *)st->right, name, module, generic);
 
   return retval;
 }
@@ -4353,8 +4349,8 @@ load_needed (pointer_info *p)
   if (p == NULL)
     return rv;
 
-  rv |= load_needed (p->left);
-  rv |= load_needed (p->right);
+  rv |= load_needed ((pointer_info *)p->left);
+  rv |= load_needed ((pointer_info *)p->right);
 
   if (p->type != P_SYMBOL || p->u.rsym.state != NEEDED)
     return rv;
@@ -4436,8 +4432,8 @@ read_cleanup (pointer_info *p)
   if (p == NULL)
     return;
 
-  read_cleanup (p->left);
-  read_cleanup (p->right);
+  read_cleanup ((pointer_info *)p->left);
+  read_cleanup ((pointer_info *)p->right);
 
   if (p->type == P_SYMBOL && p->u.rsym.state == USED && !p->u.rsym.referenced)
     {
@@ -4846,9 +4842,8 @@ gfc_check_symbol_access (gfc_symbol *sym)
 
 /* A structure to remember which commons we've already written.  */
 
-struct written_common
+struct written_common : public gfc_bbt
 {
-  BBT_HEADER(written_common);
   const char *name, *label;
 };
 
@@ -4857,29 +4852,27 @@ static struct written_common *written_commons = NULL;
 /* Comparison function used for balancing the binary tree.  */
 
 static int
-compare_written_commons (void *a1, void *b1)
+compare_written_commons (const gfc_bbt *a1, const gfc_bbt *b1)
 {
-  const char *aname = ((struct written_common *) a1)->name;
-  const char *alabel = ((struct written_common *) a1)->label;
-  const char *bname = ((struct written_common *) b1)->name;
-  const char *blabel = ((struct written_common *) b1)->label;
-  int c = strcmp (aname, bname);
+  const written_common *a = (const written_common *)a1;
+  const written_common *b = (const written_common *)b1;
+  int c = strcmp (a->name, b->name);
 
-  return (c != 0 ? c : strcmp (alabel, blabel));
+  return (c != 0 ? c : strcmp (a->label, b->label));
 }
 
 /* Free a list of written commons.  */
 
 static void
-free_written_common (struct written_common *w)
+free_written_common (written_common *w)
 {
   if (!w)
     return;
 
   if (w->left)
-    free_written_common (w->left);
+    free_written_common ((written_common *)w->left);
   if (w->right)
-    free_written_common (w->right);
+    free_written_common ((written_common *)w->right);
 
   free (w);
 }
@@ -4899,7 +4892,7 @@ write_common_0 (gfc_symtree *st, bool this_module)
   if (st == NULL)
     return;
 
-  write_common_0 (st->left, this_module);
+  write_common_0 ((gfc_symtree *)st->left, this_module);
 
   /* We will write out the binding label, or "" if no label given.  */
   name = st->n.common->name;
@@ -4915,7 +4908,7 @@ write_common_0 (gfc_symtree *st, bool this_module)
       if (c == 0)
 	write_me = false;
 
-      w = (c < 0) ? w->left : w->right;
+      w =  (written_common *)((c < 0) ? w->left : w->right);
     }
 
   if (this_module && p->use_assoc)
@@ -4946,7 +4939,7 @@ write_common_0 (gfc_symtree *st, bool this_module)
       gfc_insert_bbt (&written_commons, w, compare_written_commons);
     }
 
-  write_common_0 (st->right, this_module);
+  write_common_0 ((gfc_symtree *)st->right, this_module);
 }
 
 
@@ -5117,7 +5110,7 @@ write_symbol0 (gfc_symtree *st)
   if (st == NULL)
     return;
 
-  write_symbol0 (st->left);
+  write_symbol0 ((gfc_symtree *)st->left);
 
   sym = st->n.sym;
   if (sym->module == NULL)
@@ -5143,7 +5136,7 @@ write_symbol0 (gfc_symtree *st)
 	}
     }
 
-  write_symbol0 (st->right);
+  write_symbol0 ((gfc_symtree *)st->right);
 }
 
 
@@ -5162,7 +5155,7 @@ write_symbol1 (pointer_info *p)
   if (!p)
     return 0;
 
-  result = write_symbol1 (p->left);
+  result = write_symbol1 ((pointer_info *)p->left);
 
   if (!(p->type != P_SYMBOL || p->u.wsym.state != NEEDS_WRITE))
     {
@@ -5171,7 +5164,7 @@ write_symbol1 (pointer_info *p)
       result = 1;
     }
 
-  result |= write_symbol1 (p->right);
+  result |= write_symbol1 ((pointer_info *)p->right);
   return result;
 }
 
@@ -5201,8 +5194,8 @@ write_generic (gfc_symtree *st)
   if (st == NULL)
     return;
 
-  write_generic (st->left);
-  write_generic (st->right);
+  write_generic ((gfc_symtree *)st->left);
+  write_generic ((gfc_symtree *)st->right);
 
   sym = st->n.sym;
   if (!sym || check_unique_name (st->name))
diff --git a/gcc/fortran/parse.c b/gcc/fortran/parse.c
index 44b1900..f1a5aed 100644
--- a/gcc/fortran/parse.c
+++ b/gcc/fortran/parse.c
@@ -4405,8 +4405,8 @@ clean_up_modules (gfc_gsymbol *gsym)
   if (gsym == NULL)
     return;
 
-  clean_up_modules (gsym->left);
-  clean_up_modules (gsym->right);
+  clean_up_modules ((gfc_gsymbol *)gsym->left);
+  clean_up_modules ((gfc_gsymbol *)gsym->right);
 
   if (gsym->type != GSYM_MODULE || !gsym->ns)
     return;
diff --git a/gcc/fortran/resolve.c b/gcc/fortran/resolve.c
index ac5a362..3968d7e 100644
--- a/gcc/fortran/resolve.c
+++ b/gcc/fortran/resolve.c
@@ -953,9 +953,9 @@ resolve_common_blocks (gfc_symtree *common_root)
     return;
 
   if (common_root->left)
-    resolve_common_blocks (common_root->left);
+    resolve_common_blocks ((gfc_symtree *)common_root->left);
   if (common_root->right)
-    resolve_common_blocks (common_root->right);
+    resolve_common_blocks ((gfc_symtree *)common_root->right);
 
   resolve_common_vars (common_root->n.common->head, true);
 
@@ -11803,9 +11803,9 @@ ensure_not_abstract_walker (gfc_symbol* sub, gfc_symtree* st)
   if (!st)
     return SUCCESS;
 
-  if (ensure_not_abstract_walker (sub, st->left) == FAILURE)
+  if (ensure_not_abstract_walker (sub, (gfc_symtree *)st->left) == FAILURE)
     return FAILURE;
-  if (ensure_not_abstract_walker (sub, st->right) == FAILURE)
+  if (ensure_not_abstract_walker (sub, (gfc_symtree *)st->right) == FAILURE)
     return FAILURE;
 
   if (st->n.tb && st->n.tb->deferred)
@@ -13627,7 +13627,7 @@ warn_unused_fortran_label (gfc_st_label *label)
   if (label == NULL)
     return;
 
-  warn_unused_fortran_label (label->left);
+  warn_unused_fortran_label ((gfc_st_label *)label->left);
 
   if (label->defined == ST_LABEL_UNKNOWN)
     return;
@@ -13648,7 +13648,7 @@ warn_unused_fortran_label (gfc_st_label *label)
       break;
     }
 
-  warn_unused_fortran_label (label->right);
+  warn_unused_fortran_label ((gfc_st_label *)label->right);
 }
 
 
@@ -14143,8 +14143,8 @@ gfc_resolve_uops (gfc_symtree *symtree)
   if (symtree == NULL)
     return;
 
-  gfc_resolve_uops (symtree->left);
-  gfc_resolve_uops (symtree->right);
+  gfc_resolve_uops ((gfc_symtree *)symtree->left);
+  gfc_resolve_uops ((gfc_symtree *)symtree->right);
 
   for (itr = symtree->n.uop->op; itr; itr = itr->next)
     check_uop_procedure (itr->sym, itr->sym->declared_at);
diff --git a/gcc/fortran/symbol.c b/gcc/fortran/symbol.c
index 5a1e5ad..1c5b1aa 100644
--- a/gcc/fortran/symbol.c
+++ b/gcc/fortran/symbol.c
@@ -1921,8 +1921,8 @@ switch_types (gfc_symtree *st, gfc_symbol *from, gfc_symbol *to)
   if (sym->ts.type == BT_DERIVED && sym->ts.u.derived == from)
     sym->ts.u.derived = to;
 
-  switch_types (st->left, from, to);
-  switch_types (st->right, from, to);
+  switch_types ((gfc_symtree *)st->left, from, to);
+  switch_types ((gfc_symtree *)st->right, from, to);
 }
 
 
@@ -2090,10 +2090,10 @@ free_components (gfc_component *p)
    binary tree.  */
 
 static int
-compare_st_labels (void *a1, void *b1)
+compare_st_labels (const gfc_bbt *a1, const gfc_bbt *b1)
 {
-  int a = ((gfc_st_label *) a1)->value;
-  int b = ((gfc_st_label *) b1)->value;
+  int a = ((const gfc_st_label *) a1)->value;
+  int b = ((const gfc_st_label *) b1)->value;
 
   return (b - a);
 }
@@ -2128,8 +2128,8 @@ free_st_labels (gfc_st_label *label)
   if (label == NULL)
     return;
 
-  free_st_labels (label->left);
-  free_st_labels (label->right);
+  free_st_labels ((gfc_st_label *)label->left);
+  free_st_labels ((gfc_st_label *)label->right);
   
   if (label->format != NULL)
     gfc_free_expr (label->format);
@@ -2165,9 +2165,9 @@ gfc_get_st_label (int labelno)
 	return lp;
 
       if (lp->value < labelno)
-	lp = lp->left;
+	lp = (gfc_st_label *)lp->left;
       else
-	lp = lp->right;
+	lp = (gfc_st_label *)lp->right;
     }
 
   lp = XCNEW (gfc_st_label);
@@ -2376,12 +2376,10 @@ gfc_get_namespace (gfc_namespace *parent, int parent_types)
 /* Comparison function for symtree nodes.  */
 
 static int
-compare_symtree (void *_st1, void *_st2)
+compare_symtree (const gfc_bbt *_st1, const gfc_bbt *_st2)
 {
-  gfc_symtree *st1, *st2;
-
-  st1 = (gfc_symtree *) _st1;
-  st2 = (gfc_symtree *) _st2;
+  const gfc_symtree *st1 = (const gfc_symtree *) _st1;
+  const gfc_symtree *st2 = (const gfc_symtree *) _st2;
 
   return strcmp (st1->name, st2->name);
 }
@@ -2432,7 +2430,7 @@ gfc_find_symtree (gfc_symtree *st, const char *name)
       if (c == 0)
 	return st;
 
-      st = (c < 0) ? st->left : st->right;
+      st = (gfc_symtree *)((c < 0) ? st->left : st->right);
     }
 
   return NULL;
@@ -3072,8 +3070,8 @@ free_tb_tree (gfc_symtree *t)
   if (t == NULL)
     return;
 
-  free_tb_tree (t->left);
-  free_tb_tree (t->right);
+  free_tb_tree ((gfc_symtree *)t->left);
+  free_tb_tree ((gfc_symtree *)t->right);
 
   /* TODO: Free type-bound procedure structs themselves; probably needs some
      sort of ref-counting mechanism.  */
@@ -3091,8 +3089,8 @@ free_common_tree (gfc_symtree * common_tree)
   if (common_tree == NULL)
     return;
 
-  free_common_tree (common_tree->left);
-  free_common_tree (common_tree->right);
+  free_common_tree ((gfc_symtree *)common_tree->left);
+  free_common_tree ((gfc_symtree *)common_tree->right);
 
   free (common_tree);
 }  
@@ -3107,8 +3105,8 @@ free_uop_tree (gfc_symtree *uop_tree)
   if (uop_tree == NULL)
     return;
 
-  free_uop_tree (uop_tree->left);
-  free_uop_tree (uop_tree->right);
+  free_uop_tree ((gfc_symtree *)uop_tree->left);
+  free_uop_tree ((gfc_symtree *)uop_tree->right);
 
   gfc_free_interface (uop_tree->n.uop->op);
   free (uop_tree->n.uop);
@@ -3125,8 +3123,8 @@ free_sym_tree (gfc_symtree *sym_tree)
   if (sym_tree == NULL)
     return;
 
-  free_sym_tree (sym_tree->left);
-  free_sym_tree (sym_tree->right);
+  free_sym_tree ((gfc_symtree *)sym_tree->left);
+  free_sym_tree ((gfc_symtree *)sym_tree->right);
 
   gfc_release_symbol (sym_tree->n.sym);
   free (sym_tree);
@@ -3349,9 +3347,9 @@ count_st_nodes (const gfc_symtree *st)
   if (!st)
     return 0;
 
-  nodes = count_st_nodes (st->left);
+  nodes = count_st_nodes ((const gfc_symtree *)st->left);
   nodes++;
-  nodes += count_st_nodes (st->right);
+  nodes += count_st_nodes ((const gfc_symtree *)st->right);
 
   return nodes;
 }
@@ -3365,9 +3363,9 @@ fill_st_vector (gfc_symtree *st, gfc_symtree **st_vec, unsigned node_cntr)
   if (!st)
     return node_cntr;
 
-  node_cntr = fill_st_vector (st->left, st_vec, node_cntr);
+  node_cntr = fill_st_vector ((gfc_symtree *)st->left, st_vec, node_cntr);
   st_vec[node_cntr++] = st;
-  node_cntr = fill_st_vector (st->right, st_vec, node_cntr);
+  node_cntr = fill_st_vector ((gfc_symtree *)st->right, st_vec, node_cntr);
 
   return node_cntr;
 }
@@ -3524,7 +3522,7 @@ gfc_find_gsymbol (gfc_gsymbol *symbol, const char *name)
       if (!c)
 	return symbol;
 
-      symbol = (c < 0) ? symbol->left : symbol->right;
+      symbol = (gfc_gsymbol *)((c < 0) ? symbol->left : symbol->right);
     }
 
   return NULL;
@@ -3534,12 +3532,12 @@ gfc_find_gsymbol (gfc_gsymbol *symbol, const char *name)
 /* Compare two global symbols. Used for managing the BB tree.  */
 
 static int
-gsym_compare (void *_s1, void *_s2)
+gsym_compare (const gfc_bbt *_s1, const gfc_bbt *_s2)
 {
-  gfc_gsymbol *s1, *s2;
+  const gfc_gsymbol *s1, *s2;
 
-  s1 = (gfc_gsymbol *) _s1;
-  s2 = (gfc_gsymbol *) _s2;
+  s1 = (const gfc_gsymbol *) _s1;
+  s2 = (const gfc_gsymbol *) _s2;
   return strcmp (s1->name, s2->name);
 }
 

Re: Inheritance of gfc_symbol / gfc_component

[Mikael Morin]

On 18/08/2012 19:25, Tobias Schlüter wrote:
>>> I thought I could work around this problem without introducing a
>>> constructor by:
>>> 1) using 0 instead of -1 as value for this fake label (which is also
>>>     not a valid value for a label, so it can't collide
>>> 2) setting ST_LABEL_FORMAT = 0
>>> and then
>>> 3) not initializing at all, assuming that as for a C struct
>>>     format_asterisk would end up in .bss and thus be zero initialized.
Initialization doesn't seem to be that important after all as only the
address of the variable is used.
It would be safer, I think, to define the variable as a pointer with
invalid (non-NULL) value.

> The benefit over the
> STL's map or a hashtable (which is better suited) is that we can
> traverse a tree in defined order.  We use this property to write
> reproducible module files.
Indeed.

>> Could you post the not-yet-finished patch?
> 
> Please find it attached.  Note that two void* remain:
> gfc_{insert|delete}_bbt still need it, and I don't think there's a way
> around this without templates and without significantly changing the
> code.  As I said before, I think a change needs to have a benefit.  I'm
> disappointed by this outcome, but think this patch fails this
> requirement mainly because of the format_asterisk issue.
I'm not so much concerned about format_asterisk (see above).
My main concern is this: the increased type safety by changing the
(void*) -> (gfc_bbt*) change is balanced by the reduced type safety for
all the types inherited from gfc_bbt as the left and right pointer have
now gfc_bbt type instead of the derived type. And we better have type
safety for gfc_symtree which is used all over the place.

After digging some information on the web about C++ and casts, I'm not
even sure the casts are correct because of the following quote from the
standard:
"The order in which the base class subobjects are allocated in the most
derived object (1.8) is unspecified."
This tells me we shouldn't rely on the gfc_symtree, pointer_info,
etc having the same address as the corresponding gfc_bbt they derive
from (unless the explicit casts add/substract the necessary offset if
needed though).
We're probably safe with single inheritance, but still...

Mikael

Re: Inheritance of gfc_symbol / gfc_component

[Tobias Schlüter]

Hi Mikael,

On 2012-08-19 19:57, Mikael Morin wrote:
> My main concern is this: the increased type safety by changing the
> (void*) -> (gfc_bbt*) change is balanced by the reduced type safety for
> all the types inherited from gfc_bbt as the left and right pointer have
> now gfc_bbt type instead of the derived type. And we better have type
> safety for gfc_symtree which is used all over the place.

An interesting and valid point.  I don't think there's a way around this 
without using templates (or macros).  In a way, inheriting from gfc_bbt 
is also not really the purpose of inheritance, which is meant to express 
an "any A is also a B" relationship.  An object that can be stored in a 
tree is not the same as the tree itself.  This distinction is not made 
by my patch.  Since my interest was purely to get an equivalent 
alternative to the macro magic and void*'s, I wasn't too worried about 
this, but it's another wart.

> After digging some information on the web about C++ and casts, I'm not
> even sure the casts are correct because of the following quote from the
> standard:
> "The order in which the base class subobjects are allocated in the most
> derived object (1.8) is unspecified."
> This tells me we shouldn't rely on the gfc_symtree, pointer_info,
> etc having the same address as the corresponding gfc_bbt they derive
> from (unless the explicit casts add/substract the necessary offset if
> needed though).
> We're probably safe with single inheritance, but still...

To my knowledge it's perfectly valid to cast from a base class to a 
derived class, even though 'static_cast<>' would be the preferred C++ 
operator for the conversion.  The resulting pointer may only be 
dereferenced if the types of the objects in memory are correct, though. 
  The cast can take care of the issues with different offsets, multiple 
inheritance and so on because the compiler knows the whole class hierarchy.

Anyway, all this is academic, as the patch brings no benefit IMO.

Cheers,
- Tobi