Skip to content

Conversation

tehbone
Copy link
Contributor

@tehbone tehbone commented Oct 29, 2019

When @members is used in the lexer grammar, the content gets placed in an arbitrary location that is out of scope of the generated lexer. The template was updated so the content gets placed in the constructor, allowing for the creation of class members. This is consistent with the placement of members for parser grammars in the Javascript target.

The location of <namedActions.members> was moved be in the lexer 
constructor, like it is for the parser constructor.
@ericvergnaud
Copy link
Contributor

mmm... this is a breaking change...

@tehbone
Copy link
Contributor Author

tehbone commented Oct 29, 2019

Can you elaborate? I do realize that it could impact existing grammars that may use members for lexers in its current form, but to that end I don't believe it sets out to do what it should be doing.

@ericvergnaud
Copy link
Contributor

ericvergnaud commented Oct 29, 2019

Well one can create a member like this:
MyLexer.prototype.myMethod = function() {};
(not saying the grammar parser approach is not better)

@tehbone
Copy link
Contributor Author

tehbone commented Oct 29, 2019

I'm fine with this being rejected then. I only point out that it is an oddity that parsers have members generated in the constructor while the lexers do not.

@tehbone
Copy link
Contributor Author

tehbone commented Oct 29, 2019

One more thing to keep in mind, while
MyLexer.prototype.myMethod = function() {};
works
MyLexer.prototype.myField = new MyObject();
does not. All instantiations of MyLexer will have myField refer to the same MyObject.

@ericvergnaud
Copy link
Contributor

True.
May I suggest you run a survey on the google discussion group to find out who uses lexer members in javascript?
If nobody in 2 weeks, we can consider blessing.

@ericvergnaud
Copy link
Contributor

@parrt blessed

@parrt parrt added this to the 4.8 milestone Dec 15, 2019
@parrt parrt merged commit b8e65bf into antlr:master Dec 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants