|
View:
New views
11 Messages
—
Rating Filter:
Alert me
|
|
|
GroovyCodeVisitor and EmptyStatementsHi Gang,
The org.codehaus.groovy.ast.GroovyCodeVisitor class is used to visit AST. It has a visitX method where X is each subtype of ASTNode... except for EmptyStatement types. EmptyStatement instances will not be visited. To fix GROOVY-3840 (EmptyStatement nodes do not render in AST Browser), I could add visitEmptyStatement to GroovyCodeVisitor and the subclasses/adapters. Of course, this is backwards /incompatible/. Existing implementers would be broken. I could add visitEmptyStatement to the Adapter (CodeVisitorSupport) class that implements the GroovyCodeVisitor, which would be a less correct solution but still backwards compatible. Lastly, I could handle EmptyStatements as special cases in the AST Browser code (i think they only appear in IfStatements and TryCatchStatements). What should I do? Similar to this issue is GROOVY-3653 (CodeVisitorSupport should support visiting classes of type Variable). Nodes of type Variable are not visited, however, the Variable#initialValue field is of type Expression so it's quite likely the initial value will want to be visited by a visitor. The same questions are applicable here: Should we add visitVariable to the GroovyCodeVisitor (which is backwards incompatible), or add visitVariable to CodeVisitorSupport, or just handle nodes of type Variable as a special case in the AST browser. Again, what should I do? My preference for both these issues is to add EmptyStatements and Variables to the Adapter class (CodeVisitorSupport). Backwards compatibility is /mostly/ maintained and there is no special case code in custom visitors. By /mostly/ backwards compatible, there is the chance for defects if a user is not expected his/her visitExpressionStatement to be called for initialization of Variables. That sounds like a very unlikely edge case however. Most likely the visitor /should/ be called for Variable#initialExpression fields and isn't. Thanks, -- Hamlet D'Arcy hamletdrc@... --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatementsIf I am correct following shoud work fine
public class EmptyStatement extends ExpressionStatement { public static final EmptyStatement INSTANCE = new ExpressionStatement(new EmptyExpression()); public void visit(GroovyCodeVisitor visitor) { } public boolean isEmpty() { return true; } } On Sun, Oct 25, 2009 at 12:08 AM, Hamlet D'Arcy <hamletdrc@...> wrote: > Hi Gang, > > The org.codehaus.groovy.ast.GroovyCodeVisitor class is used to visit > AST. It has a visitX method where X is each subtype of ASTNode... > except for EmptyStatement types. EmptyStatement instances will not be > visited. > > To fix GROOVY-3840 (EmptyStatement nodes do not render in AST > Browser), I could add visitEmptyStatement to GroovyCodeVisitor and the > subclasses/adapters. Of course, this is backwards /incompatible/. > Existing implementers would be broken. I could add visitEmptyStatement > to the Adapter (CodeVisitorSupport) class that implements the > GroovyCodeVisitor, which would be a less correct solution but still > backwards compatible. Lastly, I could handle EmptyStatements as > special cases in the AST Browser code (i think they only appear in > IfStatements and TryCatchStatements). > > What should I do? > > Similar to this issue is GROOVY-3653 (CodeVisitorSupport should > support visiting classes of type Variable). Nodes of type Variable are > not visited, however, the Variable#initialValue field is of type > Expression so it's quite likely the initial value will want to be > visited by a visitor. The same questions are applicable here: Should > we add visitVariable to the GroovyCodeVisitor (which is backwards > incompatible), or add visitVariable to CodeVisitorSupport, or just > handle nodes of type Variable as a special case in the AST browser. > > Again, what should I do? > > My preference for both these issues is to add EmptyStatements and > Variables to the Adapter class (CodeVisitorSupport). Backwards > compatibility is /mostly/ maintained and there is no special case code > in custom visitors. By /mostly/ backwards compatible, there is the > chance for defects if a user is not expected his/her > visitExpressionStatement to be called for initialization of Variables. > That sounds like a very unlikely edge case however. Most likely the > visitor /should/ be called for Variable#initialExpression fields and > isn't. > > Thanks, > > -- > Hamlet D'Arcy > hamletdrc@... > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email |
|
|
Re: GroovyCodeVisitor and EmptyStatements> If I am correct following shoud work fine
I'm not sure how this answers my questions. If I use GroovyCodeVisitor to visit this: def x try { x = 1 } catch (IOException ei) { x = 2 } There is a TryCatchStatement created. That has a 'finallyStatement' field of type EmptyStatement. Groovy visitor never gets called on the EmptyStatement. -- Hamlet D'Arcy hamletdrc@... On Sun, Oct 25, 2009 at 12:41 AM, Alex Tkachman <alex.tkachman@...> wrote: > If I am correct following shoud work fine > > public class EmptyStatement extends ExpressionStatement { > > public static final EmptyStatement INSTANCE = new > ExpressionStatement(new EmptyExpression()); > > public void visit(GroovyCodeVisitor visitor) { > } > > public boolean isEmpty() { > return true; > } > > } > > > On Sun, Oct 25, 2009 at 12:08 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >> Hi Gang, >> >> The org.codehaus.groovy.ast.GroovyCodeVisitor class is used to visit >> AST. It has a visitX method where X is each subtype of ASTNode... >> except for EmptyStatement types. EmptyStatement instances will not be >> visited. >> >> To fix GROOVY-3840 (EmptyStatement nodes do not render in AST >> Browser), I could add visitEmptyStatement to GroovyCodeVisitor and the >> subclasses/adapters. Of course, this is backwards /incompatible/. >> Existing implementers would be broken. I could add visitEmptyStatement >> to the Adapter (CodeVisitorSupport) class that implements the >> GroovyCodeVisitor, which would be a less correct solution but still >> backwards compatible. Lastly, I could handle EmptyStatements as >> special cases in the AST Browser code (i think they only appear in >> IfStatements and TryCatchStatements). >> >> What should I do? >> >> Similar to this issue is GROOVY-3653 (CodeVisitorSupport should >> support visiting classes of type Variable). Nodes of type Variable are >> not visited, however, the Variable#initialValue field is of type >> Expression so it's quite likely the initial value will want to be >> visited by a visitor. The same questions are applicable here: Should >> we add visitVariable to the GroovyCodeVisitor (which is backwards >> incompatible), or add visitVariable to CodeVisitorSupport, or just >> handle nodes of type Variable as a special case in the AST browser. >> >> Again, what should I do? >> >> My preference for both these issues is to add EmptyStatements and >> Variables to the Adapter class (CodeVisitorSupport). Backwards >> compatibility is /mostly/ maintained and there is no special case code >> in custom visitors. By /mostly/ backwards compatible, there is the >> chance for defects if a user is not expected his/her >> visitExpressionStatement to be called for initialization of Variables. >> That sounds like a very unlikely edge case however. Most likely the >> visitor /should/ be called for Variable#initialExpression fields and >> isn't. >> >> Thanks, >> >> -- >> Hamlet D'Arcy >> hamletdrc@... >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> >> > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatementsIn fact, your code snippet displays the exact problem:
I want this: public void visit(GroovyCodeVisitor visitor) { } to be implemented as this: public void visit(GroovyCodeVisitor visitor) { visitor.visitEmptyStatement(this); // visitEmptyStatement does not exist today! } so that I get some sort of callback in my visitor. -- Hamlet D'Arcy hamletdrc@... On Sun, Oct 25, 2009 at 6:56 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >> If I am correct following shoud work fine > > I'm not sure how this answers my questions. > > If I use GroovyCodeVisitor to visit this: > def x > try { x = 1 } catch (IOException ei) { x = 2 } > > There is a TryCatchStatement created. That has a 'finallyStatement' > field of type EmptyStatement. Groovy visitor never gets called on the > EmptyStatement. > > > -- > Hamlet D'Arcy > hamletdrc@... > > > On Sun, Oct 25, 2009 at 12:41 AM, Alex Tkachman <alex.tkachman@...> wrote: >> If I am correct following shoud work fine >> >> public class EmptyStatement extends ExpressionStatement { >> >> public static final EmptyStatement INSTANCE = new >> ExpressionStatement(new EmptyExpression()); >> >> public void visit(GroovyCodeVisitor visitor) { >> } >> >> public boolean isEmpty() { >> return true; >> } >> >> } >> >> >> On Sun, Oct 25, 2009 at 12:08 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >>> Hi Gang, >>> >>> The org.codehaus.groovy.ast.GroovyCodeVisitor class is used to visit >>> AST. It has a visitX method where X is each subtype of ASTNode... >>> except for EmptyStatement types. EmptyStatement instances will not be >>> visited. >>> >>> To fix GROOVY-3840 (EmptyStatement nodes do not render in AST >>> Browser), I could add visitEmptyStatement to GroovyCodeVisitor and the >>> subclasses/adapters. Of course, this is backwards /incompatible/. >>> Existing implementers would be broken. I could add visitEmptyStatement >>> to the Adapter (CodeVisitorSupport) class that implements the >>> GroovyCodeVisitor, which would be a less correct solution but still >>> backwards compatible. Lastly, I could handle EmptyStatements as >>> special cases in the AST Browser code (i think they only appear in >>> IfStatements and TryCatchStatements). >>> >>> What should I do? >>> >>> Similar to this issue is GROOVY-3653 (CodeVisitorSupport should >>> support visiting classes of type Variable). Nodes of type Variable are >>> not visited, however, the Variable#initialValue field is of type >>> Expression so it's quite likely the initial value will want to be >>> visited by a visitor. The same questions are applicable here: Should >>> we add visitVariable to the GroovyCodeVisitor (which is backwards >>> incompatible), or add visitVariable to CodeVisitorSupport, or just >>> handle nodes of type Variable as a special case in the AST browser. >>> >>> Again, what should I do? >>> >>> My preference for both these issues is to add EmptyStatements and >>> Variables to the Adapter class (CodeVisitorSupport). Backwards >>> compatibility is /mostly/ maintained and there is no special case code >>> in custom visitors. By /mostly/ backwards compatible, there is the >>> chance for defects if a user is not expected his/her >>> visitExpressionStatement to be called for initialization of Variables. >>> That sounds like a very unlikely edge case however. Most likely the >>> visitor /should/ be called for Variable#initialExpression fields and >>> isn't. >>> >>> Thanks, >>> >>> -- >>> Hamlet D'Arcy >>> hamletdrc@... >>> >>> --------------------------------------------------------------------- >>> To unsubscribe from this list, please visit: >>> >>> http://xircles.codehaus.org/manage_email >>> >>> >>> >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> >> > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatementsI see.
I believe that adding such method will not break any code as long as all visitor presented in the Core will provide empty default implementation On Sun, Oct 25, 2009 at 2:05 PM, Hamlet D'Arcy <hamletdrc@...> wrote: > In fact, your code snippet displays the exact problem: > > I want this: > > public void visit(GroovyCodeVisitor visitor) { > } > > to be implemented as this: > > public void visit(GroovyCodeVisitor visitor) { > visitor.visitEmptyStatement(this); // visitEmptyStatement > does not exist today! > } > > so that I get some sort of callback in my visitor. > > > -- > Hamlet D'Arcy > hamletdrc@... > > > On Sun, Oct 25, 2009 at 6:56 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >>> If I am correct following shoud work fine >> >> I'm not sure how this answers my questions. >> >> If I use GroovyCodeVisitor to visit this: >> def x >> try { x = 1 } catch (IOException ei) { x = 2 } >> >> There is a TryCatchStatement created. That has a 'finallyStatement' >> field of type EmptyStatement. Groovy visitor never gets called on the >> EmptyStatement. >> >> >> -- >> Hamlet D'Arcy >> hamletdrc@... >> >> >> On Sun, Oct 25, 2009 at 12:41 AM, Alex Tkachman <alex.tkachman@...> wrote: >>> If I am correct following shoud work fine >>> >>> public class EmptyStatement extends ExpressionStatement { >>> >>> public static final EmptyStatement INSTANCE = new >>> ExpressionStatement(new EmptyExpression()); >>> >>> public void visit(GroovyCodeVisitor visitor) { >>> } >>> >>> public boolean isEmpty() { >>> return true; >>> } >>> >>> } >>> >>> >>> On Sun, Oct 25, 2009 at 12:08 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >>>> Hi Gang, >>>> >>>> The org.codehaus.groovy.ast.GroovyCodeVisitor class is used to visit >>>> AST. It has a visitX method where X is each subtype of ASTNode... >>>> except for EmptyStatement types. EmptyStatement instances will not be >>>> visited. >>>> >>>> To fix GROOVY-3840 (EmptyStatement nodes do not render in AST >>>> Browser), I could add visitEmptyStatement to GroovyCodeVisitor and the >>>> subclasses/adapters. Of course, this is backwards /incompatible/. >>>> Existing implementers would be broken. I could add visitEmptyStatement >>>> to the Adapter (CodeVisitorSupport) class that implements the >>>> GroovyCodeVisitor, which would be a less correct solution but still >>>> backwards compatible. Lastly, I could handle EmptyStatements as >>>> special cases in the AST Browser code (i think they only appear in >>>> IfStatements and TryCatchStatements). >>>> >>>> What should I do? >>>> >>>> Similar to this issue is GROOVY-3653 (CodeVisitorSupport should >>>> support visiting classes of type Variable). Nodes of type Variable are >>>> not visited, however, the Variable#initialValue field is of type >>>> Expression so it's quite likely the initial value will want to be >>>> visited by a visitor. The same questions are applicable here: Should >>>> we add visitVariable to the GroovyCodeVisitor (which is backwards >>>> incompatible), or add visitVariable to CodeVisitorSupport, or just >>>> handle nodes of type Variable as a special case in the AST browser. >>>> >>>> Again, what should I do? >>>> >>>> My preference for both these issues is to add EmptyStatements and >>>> Variables to the Adapter class (CodeVisitorSupport). Backwards >>>> compatibility is /mostly/ maintained and there is no special case code >>>> in custom visitors. By /mostly/ backwards compatible, there is the >>>> chance for defects if a user is not expected his/her >>>> visitExpressionStatement to be called for initialization of Variables. >>>> That sounds like a very unlikely edge case however. Most likely the >>>> visitor /should/ be called for Variable#initialExpression fields and >>>> isn't. >>>> >>>> Thanks, >>>> >>>> -- >>>> Hamlet D'Arcy >>>> hamletdrc@... >>>> >>>> --------------------------------------------------------------------- >>>> To unsubscribe from this list, please visit: >>>> >>>> http://xircles.codehaus.org/manage_email >>>> >>>> >>>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe from this list, please visit: >>> >>> http://xircles.codehaus.org/manage_email >>> >>> >>> >> > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email |
|
|
Re: GroovyCodeVisitor and EmptyStatementsAlex Tkachman schrieb:
> I see. > > I believe that adding such method will not break any code as long as > all visitor presented in the Core will provide empty default > implementation any code that will not use the default implementation, but the visitor interface will break bye blackdrag -- Jochen "blackdrag" Theodorou The Groovy Project Tech Lead (http://groovy.codehaus.org) http://blackdragsview.blogspot.com/ --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email |
|
|
Re: GroovyCodeVisitor and EmptyStatementsI have a patch that adds visitEmptyStatement support to
CodeVisitorSupport (the adapter class) and _not_ the interface. Rather than check it in, I am going to attach it to the JIRA so maybe you can look to make sure it is OK. -- Hamlet D'Arcy hamletdrc@... On Sun, Oct 25, 2009 at 8:42 AM, Jochen Theodorou <blackdrag@...> wrote: > Alex Tkachman schrieb: >> >> I see. >> >> I believe that adding such method will not break any code as long as >> all visitor presented in the Core will provide empty default >> implementation > > any code that will not use the default implementation, but the visitor > interface will break > > bye blackdrag > > -- > Jochen "blackdrag" Theodorou > The Groovy Project Tech Lead (http://groovy.codehaus.org) > http://blackdragsview.blogspot.com/ > > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatementsHamlet D'Arcy schrieb:
>> If I am correct following shoud work fine > > I'm not sure how this answers my questions. > > If I use GroovyCodeVisitor to visit this: > def x > try { x = 1 } catch (IOException ei) { x = 2 } > > There is a TryCatchStatement created. That has a 'finallyStatement' > field of type EmptyStatement. Groovy visitor never gets called on the > EmptyStatement. assuming it wouldn't have been EmptyStatement, but null, what would you have done? bye blackdrag -- Jochen "blackdrag" Theodorou The Groovy Project Tech Lead (http://groovy.codehaus.org) http://blackdragsview.blogspot.com/ --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email |
|
|
Re: GroovyCodeVisitor and EmptyStatements> assuming it wouldn't have been EmptyStatement, but null, what would you have
> done? I'd have done nothing. If an EmptyStatement is the correct value for the field, then I want the AST Browser to show that. If null is the correct value then the AST Browser does _not_ need to show that. -- Hamlet D'Arcy hamletdrc@... On Sun, Oct 25, 2009 at 8:44 AM, Jochen Theodorou <blackdrag@...> wrote: > Hamlet D'Arcy schrieb: >>> >>> If I am correct following shoud work fine >> >> I'm not sure how this answers my questions. >> >> If I use GroovyCodeVisitor to visit this: >> def x >> try { x = 1 } catch (IOException ei) { x = 2 } >> >> There is a TryCatchStatement created. That has a 'finallyStatement' >> field of type EmptyStatement. Groovy visitor never gets called on the >> EmptyStatement. > > assuming it wouldn't have been EmptyStatement, but null, what would you have > done? > > bye blackdrag > > -- > Jochen "blackdrag" Theodorou > The Groovy Project Tech Lead (http://groovy.codehaus.org) > http://blackdragsview.blogspot.com/ > > > --------------------------------------------------------------------- > To unsubscribe from this list, please visit: > > http://xircles.codehaus.org/manage_email > > > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatements>> assuming it wouldn't have been EmptyStatement, but null, what would you have
>> done? > > I'd have done nothing. However, I just checked the existing CodeVisitorSupport.java code, and it will throw NPE if the field is null. On Sun, Oct 25, 2009 at 8:47 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >> assuming it wouldn't have been EmptyStatement, but null, what would you have >> done? > > I'd have done nothing. > > If an EmptyStatement is the correct value for the field, then I want > the AST Browser to show that. If null is the correct value then the > AST Browser does _not_ need to show that. > > -- > Hamlet D'Arcy > hamletdrc@... > > > On Sun, Oct 25, 2009 at 8:44 AM, Jochen Theodorou <blackdrag@...> wrote: >> Hamlet D'Arcy schrieb: >>>> >>>> If I am correct following shoud work fine >>> >>> I'm not sure how this answers my questions. >>> >>> If I use GroovyCodeVisitor to visit this: >>> def x >>> try { x = 1 } catch (IOException ei) { x = 2 } >>> >>> There is a TryCatchStatement created. That has a 'finallyStatement' >>> field of type EmptyStatement. Groovy visitor never gets called on the >>> EmptyStatement. >> >> assuming it wouldn't have been EmptyStatement, but null, what would you have >> done? >> >> bye blackdrag >> >> -- >> Jochen "blackdrag" Theodorou >> The Groovy Project Tech Lead (http://groovy.codehaus.org) >> http://blackdragsview.blogspot.com/ >> >> >> --------------------------------------------------------------------- >> To unsubscribe from this list, please visit: >> >> http://xircles.codehaus.org/manage_email >> >> >> > -- Hamlet D'Arcy hamletdrc@... --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
|
|
Re: GroovyCodeVisitor and EmptyStatementsI attached a patch for review:
http://jira.codehaus.org/browse/GROOVY-3840 Just let me know if it looks good so I can check it in (or check in yourself, whatever). -- Hamlet D'Arcy hamletdrc@... On Sun, Oct 25, 2009 at 8:48 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >>> assuming it wouldn't have been EmptyStatement, but null, what would you have >>> done? >> >> I'd have done nothing. > > However, I just checked the existing CodeVisitorSupport.java code, > and it will throw NPE if the field is null. > > > > > On Sun, Oct 25, 2009 at 8:47 AM, Hamlet D'Arcy <hamletdrc@...> wrote: >>> assuming it wouldn't have been EmptyStatement, but null, what would you have >>> done? >> >> I'd have done nothing. >> >> If an EmptyStatement is the correct value for the field, then I want >> the AST Browser to show that. If null is the correct value then the >> AST Browser does _not_ need to show that. >> >> -- >> Hamlet D'Arcy >> hamletdrc@... >> >> >> On Sun, Oct 25, 2009 at 8:44 AM, Jochen Theodorou <blackdrag@...> wrote: >>> Hamlet D'Arcy schrieb: >>>>> >>>>> If I am correct following shoud work fine >>>> >>>> I'm not sure how this answers my questions. >>>> >>>> If I use GroovyCodeVisitor to visit this: >>>> def x >>>> try { x = 1 } catch (IOException ei) { x = 2 } >>>> >>>> There is a TryCatchStatement created. That has a 'finallyStatement' >>>> field of type EmptyStatement. Groovy visitor never gets called on the >>>> EmptyStatement. >>> >>> assuming it wouldn't have been EmptyStatement, but null, what would you have >>> done? >>> >>> bye blackdrag >>> >>> -- >>> Jochen "blackdrag" Theodorou >>> The Groovy Project Tech Lead (http://groovy.codehaus.org) >>> http://blackdragsview.blogspot.com/ >>> >>> >>> --------------------------------------------------------------------- >>> To unsubscribe from this list, please visit: >>> >>> http://xircles.codehaus.org/manage_email >>> >>> >>> >> > > > > -- > Hamlet D'Arcy > hamletdrc@... > --------------------------------------------------------------------- To unsubscribe from this list, please visit: http://xircles.codehaus.org/manage_email --
Hamlet D'Arcy |
| Free embeddable forum powered by Nabble | Forum Help |