[antlr-interest] Complaints about BaseAST implementation

Andy Tripp atripp at jazillian.com
Wed Oct 12 14:20:59 PDT 2005


Hi Akhilesh,

OK, I see what you're saying now. Split BaseAST into two: a base class
that has no reference to "down" and "right" fields, and a subclass that 
does.

I guess that makes sense. A couple of things though:
*  the subclass will need to be called BaseAST and not the base class,
or else everyone's code will break.
* You'll still want the "type" and "text" fields in the base class, I 
assume -
I can't imagine an AST that doesn't need these
* I'm still having trouble picturing why you would want to implement
these "down" and "right" fields any other way. I don't doubt that you need
to do it, I'm just saying I can't picture it. So in the end, it may not 
be worth
it for Terrence to fold in this change, because you may be the only
person who ends up needing/wanting it. I know there's no real harm in
him doing it, but it does add a little complexity.

And in defense of BaseAST (and it's author :), it's certainly not 
obvious that
someone might not want to keep "down" and "right" fields, and in fact I 
suspect
that you may be the only user who wants/needs to. So I think the
"this BaseAST design is awful" is unwarranted. Making your change may
be an improvement, but the current design has worked just fine for every 
ANTLR user
for many years, AFAICT.

Andy

Akhilesh Mritunjai wrote:

>Hi Andy
>
>--- Andy Tripp <atripp at jazillian.com> wrote:
>  
>
>>All ranting aside, I don't see how BaseAST can
>>implement AST
>>and thus provide methods like getFirstChild() and
>>getNextSibling() without
>>having any fields. Can you say specifically how that
>>can be done?
>>    
>>
>
>It doesn't have to! Thats what my point is.
>BaseAST is abstract, so there is no obligation on it
>to implement anything it has no idea about. It is the
>duty of concrete class inheriting from it to provide
>concrete implementations of set/getFirstChild() and
>set/getFirstSibling() etc. How the concrete class
>implements them is again of no concern of BaseAST. The
>contract from AST interface says they return an
>instance of type AST... so as long as it is fulfilled
>by concrete class everything should be fine.
>
>  
>
>>But BaseAST itself implements the tree
>>relationships, not
>>your subclass.
>>    
>>
>
>Thats what is wrong. BaseAST should not do it. It
>implements just the algorithms, which should have
>nothing to do with how data is *actually* stored.
>
>  
>
>>If you want to do that yourself, you
>>shouldn't
>>be subclassing BaseAST.
>>    
>>
>
>But there is problem with it. BaseAST implements a ton
>of algorithms which work just fine. They are tested
>and would work (may not be most optimally) whatever
>way data is stored. I don't want to reimplement (and
>debug) them again... I don't even want to copy/paste
>them in MyBaseAST (which will be *identical* to
>BaseAST with the two fields less).
>
>So eg, addChild() would work fine even if childs are
>stored in a doubly linked list, however, if the
>implementer wants better performance she/he might
>choose to implement it in fashion that gives O(1) 
>performance rather than O(n) which is right now.
>Again, it is implementer's choice.
>
>- Akhilesh
>
>
>
>		
>__________________________________ 
>Yahoo! Music Unlimited 
>Access over 1 million songs. Try it free.
>http://music.yahoo.com/unlimited/
>
>  
>


More information about the antlr-interest mailing list