Pull #4102: changed loops to end execution early #4102

Merged
merged 1 commit into from Apr 5, 2017

Conversation

Projects
None yet
4 participants
@rnveach
Member

rnveach commented Mar 26, 2017

When scanning Checkstyle's code for a failed check, I found the following places where we had a loop looking for something and assigning a variable if it was found or not. I noticed most of the loops were specifically not ending when a determination was found.

I changed spots involving loops and assigning variables true/false to end their execution sooner as the variable is modified.

Changes to InputIllegalTypeMemberModifiers were to keep code coverage at 100% for the check as we lost some on the for loop for the fixes.

appveyor required change to JavadocMethodCheck that it be changed to a for-each.

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Mar 26, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@753537b). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4102   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?     283           
  Lines             ?   14804           
  Branches          ?    3380           
========================================
  Hits              ?   14804           
  Misses            ?       0           
  Partials          ?       0
Impacted Files Coverage Δ
...ols/checkstyle/checks/design/ThrowsCountCheck.java 100% <100%> (ø)
...ls/checkstyle/checks/javadoc/JavadocTypeCheck.java 100% <100%> (ø)
...ools/checkstyle/checks/SuppressWarningsHolder.java 100% <100%> (ø)
...puppycrawl/tools/checkstyle/utils/CommonUtils.java 100% <100%> (ø)
.../checkstyle/checks/javadoc/JavadocMethodCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/IllegalTypeCheck.java 100% <100%> (ø)
...yle/checks/whitespace/NoWhitespaceBeforeCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/MagicNumberCheck.java 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 753537b...ed9c24d. Read the comment docs.

codecov-io commented Mar 26, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@753537b). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #4102   +/-   ##
========================================
  Coverage          ?    100%           
========================================
  Files             ?     283           
  Lines             ?   14804           
  Branches          ?    3380           
========================================
  Hits              ?   14804           
  Misses            ?       0           
  Partials          ?       0
Impacted Files Coverage Δ
...ols/checkstyle/checks/design/ThrowsCountCheck.java 100% <100%> (ø)
...ls/checkstyle/checks/javadoc/JavadocTypeCheck.java 100% <100%> (ø)
...ools/checkstyle/checks/SuppressWarningsHolder.java 100% <100%> (ø)
...puppycrawl/tools/checkstyle/utils/CommonUtils.java 100% <100%> (ø)
.../checkstyle/checks/javadoc/JavadocMethodCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/RequireThisCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/IllegalTypeCheck.java 100% <100%> (ø)
...yle/checks/whitespace/NoWhitespaceBeforeCheck.java 100% <100%> (ø)
...ols/checkstyle/checks/coding/MagicNumberCheck.java 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 753537b...ed9c24d. Read the comment docs.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 26, 2017

Member

appveyor required change to JavadocMethodCheck that it be changed to for each.

Member

rnveach commented Mar 26, 2017

appveyor required change to JavadocMethodCheck that it be changed to for each.

@rnveach rnveach requested a review from romani Mar 26, 2017

@@ -236,6 +236,7 @@ private boolean isContainVerifiableType(DetailAST modifiers) {
modifier = modifier.getNextSibling()) {

This comment has been minimized.

@MEZk

MEZk Mar 26, 2017

Contributor

This can be replaced with TokenUtils#findFirstTokenByPredicate, but it is up to you to decide.

@MEZk

MEZk Mar 26, 2017

Contributor

This can be replaced with TokenUtils#findFirstTokenByPredicate, but it is up to you to decide.

This comment has been minimized.

@rnveach

rnveach Mar 27, 2017

Member

If @romani is fine with this I'll make the change.

@rnveach

rnveach Mar 27, 2017

Member

If @romani is fine with this I'll make the change.

This comment has been minimized.

@romani

romani Mar 27, 2017

Member

please do not do all optimizations in one PR. Split this request to separate issue

@romani

romani Mar 27, 2017

Member

please do not do all optimizations in one PR. Split this request to separate issue

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Mar 27, 2017

Member

romani has requested I change this to a full PR issue and provide regression.

Member

rnveach commented Mar 27, 2017

romani has requested I change this to a full PR issue and provide regression.

@rnveach rnveach changed the title from minor: changed to end execution earlier to Pull #4102: changed loops to end execution early Mar 28, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
Member

rnveach commented Apr 3, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 4, 2017

Member

Here is the second half of the report: http://rveach.no-ip.org/checkstyle/regression/reports/11/
No differences.

Member

rnveach commented Apr 4, 2017

Here is the second half of the report: http://rveach.no-ip.org/checkstyle/regression/reports/11/
No differences.

@MEZk

This comment has been minimized.

Show comment
Hide comment
@MEZk

MEZk Apr 4, 2017

Contributor

@romani
Reports are OK, let's merge this fix.

Contributor

MEZk commented Apr 4, 2017

@romani
Reports are OK, let's merge this fix.

@romani romani merged commit d4bd21d into checkstyle:master Apr 5, 2017

4 of 7 checks passed

codecov/patch No report found to compare against
Details
codecov/project No report found to compare against
Details
IDEA Inspections Pull Request (Checkstyle) TeamCity build failed
Details
codacy/pr Good work! A positive pull request.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
wercker/build Wercker pipeline passed
Details

@romani romani added the miscellaneous label Apr 5, 2017

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

@rnveach rnveach deleted the rnveach:end_execution_sooner branch Apr 5, 2017

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