API Design: AstNodeCreator

May 18, 2009 at 8:25 PM

I think AstNodeCreator is a great thing, and it is already solving me a lot of effort.

However, when I was only starting to use it I noticed two not-obvious thing that make it a bit harder to use than necessary:

  1. The fact that I have to do grammar.SetLanguageFlags(LanguageFlags.CreateAst, true).
    It would be nicer to have a grammar.ShouldCreateAst property.
    And if AstNodeCreator is set, but ShouldCreateAst is not, maybe set AstNode to some constant value containing the meaningful message ("Please set ShouldCreateAst if you want AstNodes to be created")?
  2. Lambda expressions allow writing expressions inside Action delegates, so I originally wrote node creators as returning AstNodes and noticed the problem only the hard way.
    It might be more obvious to expect a AstNode to be returnedfrom AstNodeCreator instead of relying on it to set the node.

Coordinator
May 19, 2009 at 5:31 AM

hi

First of all - thanks for finding this inconsistency with lists node creation, will fix it asap

Now about your other "troubles"

1. So, you suggest to create both boolean property and a flag in language flags, just to catch when programmer set one and forgot another? I really think that's would seem a bit "strange". As for creating just a bool property, I would prefer to join all such bool props in this LanguageFlags set. You had trouble because you initially missed that you need to set this flag - so, it's one in a life-time trouble. It's because previously Irony was always creating AST tree - because it was the only output from parser; so without AST parsing was pointless. Now it changed, parser produces parse tree, and optionally AST. You had trouble just because of this change of behavior. If you started with Irony as it is now, that wouldn't happen - I guess.

2. About node creators returning new nodes, instead of setting the field in ParseTreeNode - it might be more obvious, but still somebody would ask "why return if I can set it right there? what you do with returned node? don't you set it? What if I set it and return?" - i'm afraid it might raise questions as well. I'm saying neither way is perfect, but the current convention seems bares a little less risk for misunderstanding.

thanks

Roman 

 

May 19, 2009 at 6:24 AM

Hi,

  1. No, I was saying to create a property that wraps the flag and sets it, just for convenience.
    I understand that it creates two different ways to do a thing, but flags are less obvious then common properties.
    You already have MarkTransient, for example, which also just sets a flag.

    As for catching, it was the first time for me, but I actually had to debug into the internals of Irony to find out what's happening.
    I think setting AstNode to something explaining why it is empty would be much better than leaving it null.
    (Also since the explanation would be same for all nodes it would not introduce any performance loss).

  2. Well, it is named 'Creator', so it seems logical to create a node and return it.
    If it was 'Setter' or 'Initializer', I would expect that I have to set node, but not with 'Creator'.
    Also it would remove the code "node.AstNode =" which has to be written over and over again.

Now, I do understand that these are first time problems and they may exist only in my mind anyway (that's why it is a discussion, not an issue).
But I think that any API should try to prevent common errors and reduce repetitive coding, so that's why I am bringing this up.

Andrey

Coordinator
May 19, 2009 at 4:49 PM

Hi

1. As for MarkTransient, the main reason for it is not to duplicate a flag-setting approach, but to allow set this flag on multiple non-terminals in one statement; there's no such situation here. I do agree that currently this node-creation flag is kinda hidden and not obvious. I think that would become much better once there are XML comments, so once you start setting NodeType or NodeCreator properties, you'll eventually read from xml doc that using these properties requires setting CreateAst flag on grammar. What I can do is the following: add validation at grammar analysis - check that if any of the NodeType or NodeCreator properties are set on any of the non-terminals, but CreateAst flag is not set, then add warning to grammar errors list. Will put it on my list

2. Yeah, it is named Creator and it might be suggesting to be a function. But from signature you see it is not a function, so it would probably lead to the question where to put the node, and eventually the programmer would find out the Node field; call it Initializer - that would be confusing: initializer does not create stuff, it initializes existing thing. Again, I do agree there maybe some confusion out in existing arrangement, but I don't think alternatives are better in this regard.

Thanks for the input, let's continue on this path, to make the experience better

Roman

Coordinator
May 22, 2009 at 9:28 PM

I've fixed the AST node creation for lists, added warning as I describe in #1

Try it, let me know if it works correctly

thanks

Roman

May 23, 2009 at 6:10 PM

Thanks, I hope this helps people. I'll check it after I solve my other problems.