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

EmptyBlock should process LITERAL_DEFAULT #4159

Closed
romani opened this Issue Apr 5, 2017 · 7 comments

Comments

Projects
None yet
3 participants
@romani
Member

romani commented Apr 5, 2017

from pull #3918 discussion.

Looks like we have the same problem for DEFAULT_TOKEN. I think that it should be fixed in separate issue.

$ cat TestClass.java
public class TestClass {
        void method(int a) {
            switch (a) {}
            switch (a) {default: }
            switch (a) {default: {}} // violation is expected
            switch (a) {
                default:
            }
            switch (a) {
                default: // violation is expected
                {}
            }
            switch (a) {
                default: // violation is expected
                {
                }
            }
        }
    }

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>
<module name="TreeWalker">
        <module name="EmptyBlock">
            <property name="tokens" value="LITERAL_DEFAULT"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-7.6-all.jar -c TestConfig.xml TestClass.java
Starting audit...
Audit done.
@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Apr 5, 2017

Collaborator

I am on it

Collaborator

ps-sp commented Apr 5, 2017

I am on it

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Apr 7, 2017

Collaborator

@romani @rnveach @MEZk Does it benefit to separately check for empty case and empty default token ? If not perhaps we can remove empty case and empty default from the list of acceptable tokens and just add CASE_GROUP token. But like I mentioned, we won't be then able to check for empty LITERAL_CASE and empty LITERAL_DEFAULT separately. Please address. Thankyou.

Collaborator

ps-sp commented Apr 7, 2017

@romani @rnveach @MEZk Does it benefit to separately check for empty case and empty default token ? If not perhaps we can remove empty case and empty default from the list of acceptable tokens and just add CASE_GROUP token. But like I mentioned, we won't be then able to check for empty LITERAL_CASE and empty LITERAL_DEFAULT separately. Please address. Thankyou.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 9, 2017

Member

@ps-sp ,

But like I mentioned,

please share a link , where you prove this.

Member

romani commented Apr 9, 2017

@ps-sp ,

But like I mentioned,

please share a link , where you prove this.

@ps-sp

This comment has been minimized.

Show comment
Hide comment
@ps-sp

ps-sp Apr 10, 2017

Collaborator

@romani I think you misunderstood !

IN SHORT
Should we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP to the list of acceptable tokens ? It will help us detect both the tokens at the same time.

What I said was:

If not perhaps we can remove empty case and empty default from the list of acceptable tokens and just add CASE_GROUP token.

and before that I asked this question:

Does it benefit to separately check for empty case and empty default token ?

What I meant was we won't be able to check for empty LITERAL_CASE and empty LITERAL_DEFAULT "SEPARATELY" if we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP. They would be checked for together in the CASE_GROUP.

Collaborator

ps-sp commented Apr 10, 2017

@romani I think you misunderstood !

IN SHORT
Should we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP to the list of acceptable tokens ? It will help us detect both the tokens at the same time.

What I said was:

If not perhaps we can remove empty case and empty default from the list of acceptable tokens and just add CASE_GROUP token.

and before that I asked this question:

Does it benefit to separately check for empty case and empty default token ?

What I meant was we won't be able to check for empty LITERAL_CASE and empty LITERAL_DEFAULT "SEPARATELY" if we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP. They would be checked for together in the CASE_GROUP.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 10, 2017

Contributor

@ps-sp @romani

Does it benefit to separately check for empty case and empty default token ?

From my point of view, there is no need in checking LITERAL_CASE and LITERAL_DEFAULT separately.
However, it might be a problem for the user to understand that CASE_GROUP is for case and default statements.

Contributor

MEZk commented Apr 10, 2017

@ps-sp @romani

Does it benefit to separately check for empty case and empty default token ?

From my point of view, there is no need in checking LITERAL_CASE and LITERAL_DEFAULT separately.
However, it might be a problem for the user to understand that CASE_GROUP is for case and default statements.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 11, 2017

Member

Should we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP to the list of acceptable tokens ? It will help us detect both the tokens at the same time.

No, you never know user preferences. This Check do validation of first children in tree, it should continue to do this. Check already do this for IF and ELSE separately. Consistency is better.

Member

romani commented Apr 11, 2017

Should we remove LITERAL_CASE and LITERAL_DEFAULT from the list of acceptable tokens and just add CASE_GROUP to the list of acceptable tokens ? It will help us detect both the tokens at the same time.

No, you never know user preferences. This Check do validation of first children in tree, it should continue to do this. Check already do this for IF and ELSE separately. Consistency is better.

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 11, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 11, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 11, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 12, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 13, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 16, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 16, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 16, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 16, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 17, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 18, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 18, 2017

ps-sp added a commit to ps-sp/checkstyle that referenced this issue Apr 19, 2017

romani added a commit that referenced this issue Apr 21, 2017

@romani romani added the bug label Apr 21, 2017

@romani romani added this to the 7.7 milestone Apr 21, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Apr 21, 2017

Member

fix is merged

Member

romani commented Apr 21, 2017

fix is merged

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