Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Double package declaration in Visitor-Interface #30

Merged
merged 1 commit into from Aug 6, 2020

Conversation

silasmahler
Copy link
Contributor

The generated Visitor-Interface from Visitor.java.ftl has a doubled package declaration when used with the PARSER_PACKAGE option in the .jjt-file.
I assume it's caused by the missing "elseif" which is for example present in the template for BaseNode, but this would need to be tested, because I know too less about the internas atm.

The generated Visitor-Interface from Visitor.java.ftl has a doubled package declaration when used with the  PARSER_PACKAGE option in the .jjt-file.
I assume it's caused by the missing "elseif" which is for example present in the template for BaseNode, but this would need to be tested, because I know too less about the internas atm.
@revusky revusky merged commit 50010c3 into javacc21:master Aug 6, 2020
@revusky
Copy link
Member

revusky commented Aug 6, 2020

I need to make a comment or two about this. I merged it, but I think the real solution moving forward is just to get rid of all this legacy Visitor stuff from the old JJTree days. All of that was designed somewhere around 1997 and never really revisited, you know. That was at a point in time before there was generics and possibly even before there was reflection in Java.

I have another Visitor implementation, Node.Visitor, which maybe can be improved, but even so, I think it's much more usable, less cumbersome than that old JJTree approach. But the funny thing about all this is that I had probably been intending to get rid of that, but it was still around. If you type java -jar javacc.jar on the command line, and get the usage info, it doesn't even mention these options like VISITOR_RETURN_TYPE and things like this.

The other aspect of all of this is that I was keeping this old stuff around, but due to code drift over time, it wasn't even working correctly. Probably this was the case for a long time, but since I don't actually use that myself and don't have any test cases that use it... OTOH, the Node.Visitor stuff is used internally, so it can't just stop working! What is very cumbersome about this legacy approach is that it generates this interface that doesn't have any default implementation, so you always have to write stubs for every last one of them by hand. Well, come to think of it, since Java now allows you to put default implementations in an interface (since java 7 or 8 or I dunno) the template could be kept around and we could have it generate the default implementation as well.

But, look, what I would propose is that you play around with the Node.Visitor that I wrote later to supersede all this really, and see if you think there is much of a case for keeping that older stuff around. If not, I think it should just be eliminated. The fact that in a period of some months, nobody even told me (before you) that this was broken kind of suggests that nobody would miss it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants