Remove unreachable code in OrderedPeriodParser #24405
Conversation
| } | ||
|
|
||
| return ~bestInvalidPos; |
There was a problem hiding this comment.
This line is unreachable because bestValidPos > position || (bestValidPos == position) is always true.
| // Restore the state to the best valid parse. | ||
| if (bestValidPeriod != null) { | ||
| period.setPeriod(bestValidPeriod); | ||
| return ~parsePos; |
There was a problem hiding this comment.
I think there is no reason to continue trying to parse when even one of parsers fails in this use case. It would be better to return earlier.
There was a problem hiding this comment.
I think the reason it is there is because it can provide better error messages since the error that would be retained would be from the "longest matching" parser.
There was a problem hiding this comment.
In that case, I wonder how it should work when any parsers fail. Just throwing away error position and returning the longest succeeded position (or the initial position if there are no successful parsers) is expected?
Sure, that works because PeriodFormatter throws an exception when returned position is less than the text length: https://github.com/JodaOrg/joda-time/blob/v2.12.7/src/main/java/org/joda/time/format/PeriodFormatter.java#L323-L331
There was a problem hiding this comment.
I revised this PR to just remove the unreachable (and meaningless) code.
takezoe
force-pushed
the
return-datetime-parse-failure-earlier
branch
from
b7afe3d to
80bd9a6
Compare
takezoe
changed the title
takezoe
force-pushed
the
return-datetime-parse-failure-earlier
branch
from
80bd9a6 to
ba07989
Compare
Description
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: