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

reevaluate 'default' and 'case' in google config for EmptyBlock #3748

Closed
rnveach opened this Issue Jan 23, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jan 23, 2017

Taken from PR #3743 ,

The tokens default and case shouldn't be empty according to http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.4-switch .
We should review and decide if tokens should be added to google configuration.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 26, 2017

Member

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.4-switch

Each statement group consists of one or more switch labels (either case FOO: or default:), followed by one or more statements.

4.8.4.3

Each switch statement includes a default statement group, even if it contains no code.

default clearly says it may contain no code. case says the group must contain one or more statements, but it also stated default in that same sentence.

@romani We can try asking for confirmation, but it seems case should be added and default shouldn't be.

Member

rnveach commented Jan 26, 2017

http://checkstyle.sourceforge.net/reports/google-java-style-20160712.html#s4.8.4-switch

Each statement group consists of one or more switch labels (either case FOO: or default:), followed by one or more statements.

4.8.4.3

Each switch statement includes a default statement group, even if it contains no code.

default clearly says it may contain no code. case says the group must contain one or more statements, but it also stated default in that same sentence.

@romani We can try asking for confirmation, but it seems case should be added and default shouldn't be.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jan 26, 2017

Member

@rnveach , lets make issue on google style and request confirmation.

Member

romani commented Jan 26, 2017

@rnveach , lets make issue on google style and request confirmation.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Jan 26, 2017

Member

Requested confirmation at google/styleguide#221

Member

rnveach commented Jan 26, 2017

Requested confirmation at google/styleguide#221

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

@romani Its been 19 days and no response. No one has modified style since December.
I suggest we go with my recommendation and if it is wrong, someone will contact us.

Member

rnveach commented Feb 13, 2017

@romani Its been 19 days and no response. No one has modified style since December.
I suggest we go with my recommendation and if it is wrong, someone will contact us.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 13, 2017

Member

@romani Trying to add examples for case and I can't get a violation.
Is this a bug or is this because cases don't require {}s which the code seems to be looking for?

$ cat TestClass.java
public class TestClass {
    void method(int a) {
                switch (a) {}
                switch (a) {case 1:}
                switch (a) {case 1:{}}
                switch (a) {
                    case 1:
                }
                switch (a) {
                    case 1:
                    {}
                }
                switch (a) {
                    case 1:
{
}
                }
    }
}

$ 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_SWITCH, LITERAL_CASE"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-7.5.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3:28: Must have at least one statement. [EmptyBlock]
Audit done.
Checkstyle ends with 1 errors.

Only empty switch has violation, not empty cases.
Tests for check makes no reference to cases. https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/EmptyBlockCheckTest.java

Member

rnveach commented Feb 13, 2017

@romani Trying to add examples for case and I can't get a violation.
Is this a bug or is this because cases don't require {}s which the code seems to be looking for?

$ cat TestClass.java
public class TestClass {
    void method(int a) {
                switch (a) {}
                switch (a) {case 1:}
                switch (a) {case 1:{}}
                switch (a) {
                    case 1:
                }
                switch (a) {
                    case 1:
                    {}
                }
                switch (a) {
                    case 1:
{
}
                }
    }
}

$ 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_SWITCH, LITERAL_CASE"/>
        </module>
    </module>
</module>

$ java -jar checkstyle-7.5.1-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3:28: Must have at least one statement. [EmptyBlock]
Audit done.
Checkstyle ends with 1 errors.

Only empty switch has violation, not empty cases.
Tests for check makes no reference to cases. https://github.com/checkstyle/checkstyle/blob/master/src/test/java/com/puppycrawl/tools/checkstyle/checks/blocks/EmptyBlockCheckTest.java

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 17, 2017

Member

empty block in "case" is nonsense to use/have in code, it is kind of expected that guide does not clarify that point.

technically, fact that checkstyle does not report problems on this is a bug.
But in real life I think it is almost impossible to find.

Member

romani commented Feb 17, 2017

empty block in "case" is nonsense to use/have in code, it is kind of expected that guide does not clarify that point.

technically, fact that checkstyle does not report problems on this is a bug.
But in real life I think it is almost impossible to find.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 17, 2017

Member

technically, fact that checkstyle does not report problems on this is a bug.

I will move the case to a new issue.

This issue will have to wait until the problem with the check is fixed.

Member

rnveach commented Feb 17, 2017

technically, fact that checkstyle does not report problems on this is a bug.

I will move the case to a new issue.

This issue will have to wait until the problem with the check is fixed.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Feb 17, 2017

Member

After some clarification from the new issue, EmptyBlock with case/default can't satisfy any google requirements as the brace must be written in. Google makes no mentions of braces in this scenario.

We will ignore both tokens for now.

Member

rnveach commented Feb 17, 2017

After some clarification from the new issue, EmptyBlock with case/default can't satisfy any google requirements as the brace must be written in. Google makes no mentions of braces in this scenario.

We will ignore both tokens for now.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Feb 18, 2017

Member

fix is merged.

Member

romani commented Feb 18, 2017

fix is merged.

@romani romani closed this Feb 18, 2017

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