update documentation for VariableDeclarationUsageDistance with allowedDistance = 0 #3665

Closed
vanniktech opened this Issue Dec 18, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@vanniktech

Given this check is activated:

<module name="VariableDeclarationUsageDistance">
  <property name="allowedDistance" value="0"/>
</module>

it'll fail on this code:

long result = dateTime != null ? dateTime.hashCode() : 0L;
result = 31L * result + (shift != null ? shift.hashCode() : 0L);
return result;

with this error message:

Distance between variable 'result' declaration and its first usage is 1, but allowed 0. Consider to make that variable as final if you still need to store its value in advance (before method calls that might do side effect on original value). [VariableDeclarationUsageDistance]

Is this really wanted? I'd expect it not to fail since the next usage is really one line below it.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 18, 2016

Member

@vanniktech I believe 0 is an invalid value. It basically says all variables can only reference itself. See example below.
Try using a value of 1 instead of 0.

$ cat TestClass.java
public class TestClass {
    long method() {
long result = dateTime != null ? dateTime.hashCode() : 0L;
return result;
    }
}

$ 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="VariableDeclarationUsageDistance">
  <property name="allowedDistance" value="0"/>
</module>
    </module>
</module>

$ java -jar checkstyle-7.3-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3: Distance between variable 'result' declaration and its first usage is 1, but allowed 0.  Consider to make that variable as final if you still need to store its value in advance (before method calls that might do side effect on original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 1 errors.
Member

rnveach commented Dec 18, 2016

@vanniktech I believe 0 is an invalid value. It basically says all variables can only reference itself. See example below.
Try using a value of 1 instead of 0.

$ cat TestClass.java
public class TestClass {
    long method() {
long result = dateTime != null ? dateTime.hashCode() : 0L;
return result;
    }
}

$ 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="VariableDeclarationUsageDistance">
  <property name="allowedDistance" value="0"/>
</module>
    </module>
</module>

$ java -jar checkstyle-7.3-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3: Distance between variable 'result' declaration and its first usage is 1, but allowed 0.  Consider to make that variable as final if you still need to store its value in advance (before method calls that might do side effect on original value). [VariableDeclarationUsageDistance]
Audit done.
Checkstyle ends with 1 errors.
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 18, 2016

Member

value 0 does not make any sense, unfortunately it is not described in our documentation - http://checkstyle.sourceforge.net/config_coding.html#VariableDeclarationUsageDistance

in scope of this bug, documentation need to be updated.

Member

romani commented Dec 18, 2016

value 0 does not make any sense, unfortunately it is not described in our documentation - http://checkstyle.sourceforge.net/config_coding.html#VariableDeclarationUsageDistance

in scope of this bug, documentation need to be updated.

@romani romani changed the title from VariableDeclarationUsageDistance trips over hashCode implementations when allowedDistance = 0 to update documentation for VariableDeclarationUsageDistance with allowedDistance = 0 Dec 18, 2016

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Dec 18, 2016

Member

Since the property is a numeric field, it allows negative numbers too.
When we update documentation, it should be to discourage using anything less than 1.

Member

rnveach commented Dec 18, 2016

Since the property is a numeric field, it allows negative numbers too.
When we update documentation, it should be to discourage using anything less than 1.

@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Dec 18, 2016

Updating the documentation sounds good to me. Might be even worth forbidding negative values.

Updating the documentation sounds good to me. Might be even worth forbidding negative values.

romani added a commit that referenced this issue Dec 24, 2016

@romani romani added this to the 7.4 milestone Dec 24, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 24, 2016

Member

doc is extended.
issue is closed

Member

romani commented Dec 24, 2016

doc is extended.
issue is closed

@romani romani closed this Dec 24, 2016

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