Issue #669: removed branchContains for complicated cases (part 1)

[rnveach]

Issue #669

Regression to come.

[coveralls]

Coverage remained the same at 98.761% when pulling d6f3878 on rnveach:issue_669_2 into b161d27 on sevntu-checkstyle:master.

[rnveach]

Regression: http://rveach.no-ip.org/checkstyle/regression/reports/159/

Seems there are differences with AvoidConstantAsFirstOperandInConditionCheck and PublicReferenceToPrivateTypeCheck. I'll need to look into and fix these.

This can't be merged yet.

[romani]

ones regression is resolved - ok to merge.

[rnveach]

@romani
For AvoidConstantAsFirstOperandInCondition, to me it looks like these new violations are valid.

http://rveach.no-ip.org/checkstyle/regression/reports/159/guava/index.html#A1
http://rveach.no-ip.org/checkstyle/regression/reports/159/guava/xref/guava/src/com/google/common/base/SmallCharMatcher.java.html#L62

return 1 == (1 & (filter >> c));

1 should be on the right side as it is a constant. It seems the majority of the violations follow this pattern. I will verify them all after I check the other check.
You agree? If so, I'll split it into a new issue.

[romani]

Yes, violation is valid.
If it valid , why new issue is required ?

[rnveach]

I just thought it should be more documented on valid changes in logic. If issue arises X years down the road from this change, won't we assume it is something we missed in review and not expected since it is grouped with non-logic change?
I don't think we did this in main project, so I guess it can stay here.

[rnveach]

Issues for PublicReferenceToPrivateType also look valid.
http://rveach.no-ip.org/checkstyle/regression/reports/159/openjdk10/index.html#A2
http://rveach.no-ip.org/checkstyle/regression/reports/159/openjdk10/xref/src/java.base/share/classes/java/lang/invoke/MethodType.java.html#L199 - violation
http://rveach.no-ip.org/checkstyle/regression/reports/159/openjdk10/xref/src/java.base/share/classes/java/lang/invoke/MethodType.java.html#L1266 - inner class
http://rveach.no-ip.org/checkstyle/regression/reports/159/openjdk10/xref/src/java.base/share/classes/java/lang/invoke/MethodType.java.html#L1330 - nested inner class that extends another class

It looks like branchContains was going too deep and went from inner class to secondary inner class to get extends. Most of check only looks for private modifiers and this field is package-private so it can escape the class. Since this check is all about not allowing escape outside itself, this seems valid violation.
Agree?

[rnveach]

I will add cases as new tests and confirm the rest of the violations.

[rnveach]

I added new tests and confirmed all new violations.
Confirm violation for PublicReferenceToPrivateType and this can be merged.

[romani]

won't we assume it is something we missed in review

most likely but regression report generation is recent practice, such Checks far before.

http://rveach.no-ip.org
Agree ?

service is not available.

[rnveach]

Storm in my area so I turned it off for safety. It should be coming up now.

[romani]

http://rveach.no-ip.org/checkstyle/regression/reports/159/openjdk10/index.html#A2

valid violations, look like we can file issue against openjdk.
ok to merge.