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

Remove extra numeric offset in JavadocTokenTypes #5114

Closed
romani opened this Issue Sep 15, 2017 · 6 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented Sep 15, 2017

RULE_TYPES_OFFSET constant is used to split lexer tokens types and parser rules types.
We need unique numbers for all tokens, ANTLR do not need this and that is why his types are mixed by used values. All values we can take a look at target/generated-sources/antlr/com/puppycrawl/tools/checkstyle/grammars/javadoc/JavadocParser.java
for example: LEADING_ASTERISK=1 and RULE_htmlElement = 1.
RULE_TYPES_OFFSET required to shift parser rules, to let them not overlap with types that have prefix "RULE_".

cases:

    public static final int HTML_ELEMENT_START = JavadocParser.RULE_htmlElementStart
            + RULE_TYPES_OFFSET
+ RULE_TYPES_OFFSET;
    public static final int ATTRIBUTE = JavadocParser.RULE_attribute
            + RULE_TYPES_OFFSET
+ RULE_TYPES_OFFSET;
    public static final int HTML_COMMENT = JavadocParser.RULE_htmlComment
            + RULE_TYPES_OFFSET
+ RULE_TYPES_OFFSET;

ToDo:

  1. extent javadoc of RULE_TYPES_OFFSET by description that I gave in first paragraph
  2. remove extra "+ RULE_TYPES_OFFSET" from that 3 cases.

This issue might result in braking compatibility and even weird behavior of custom checks which used that 3 token types in their code. As static final int is in-lined to custom Check code during compilation.
Workaround is to recompile custom Checks(their jars).

Example of breaking compatibility: #505

Migration notes for users:
users have to recompile their extensions to work without problems, as such constants are inlined in their code. So Checks might not receive proper call on token.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 15, 2017

Member

@ps-sp , do you see any problems with removal of extra "+ RULE_TYPES_OFFSET" ?

Member

romani commented Sep 15, 2017

@ps-sp , do you see any problems with removal of extra "+ RULE_TYPES_OFFSET" ?

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Sep 15, 2017

Collaborator

@romani I can not see any problem with it. Actually, we can utilize the Vocabulary class provided by ANTLR. A public static final Vocablury instance is created in the ANTLR target sources, i.e JavadocParser.java, that has all the units from ANTLR grammar. That way we won't even need the static block in JavadocUtils. I didn't create an issue for this yet as I thought I would first work it out and check the feasibility of the workaround. We can discuss this further, should I create an issue ?

Collaborator

ps-sp commented Sep 15, 2017

@romani I can not see any problem with it. Actually, we can utilize the Vocabulary class provided by ANTLR. A public static final Vocablury instance is created in the ANTLR target sources, i.e JavadocParser.java, that has all the units from ANTLR grammar. That way we won't even need the static block in JavadocUtils. I didn't create an issue for this yet as I thought I would first work it out and check the feasibility of the workaround. We can discuss this further, should I create an issue ?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 15, 2017

Member

Thanks a lot, please create separate issue on this.

Member

romani commented Sep 15, 2017

Thanks a lot, please create separate issue on this.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 16, 2017

Member

point "1)" is done - dc4bec8

Member

romani commented Sep 16, 2017

point "1)" is done - dc4bec8

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Sep 16, 2017

Collaborator

@romani

please create separate issue on this.

#5119

Collaborator

ps-sp commented Sep 16, 2017

@romani

please create separate issue on this.

#5119

umkamax added a commit to epam/checkstyle that referenced this issue Oct 18, 2017

umkamax added a commit to epam/checkstyle that referenced this issue Oct 18, 2017

umkamax added a commit to epam/checkstyle that referenced this issue Oct 18, 2017

romani added a commit that referenced this issue Oct 20, 2017

@romani romani added this to the 8.4 milestone Oct 20, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Oct 20, 2017

Member

Fix is merged

Member

romani commented Oct 20, 2017

Fix is merged

@romani romani closed this Oct 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment