Re: Inheritance of gfc_symbol / gfc_component

[Tobias Schlüter <tobias dot schlueter at physik dot uni-muenchen dot de>]

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: patch_bbt.diff.txt
Description: Text document