On Tue, 28 Feb 2023 13:31:56 GMT, Raffaello Giulietti <[email protected]>
wrote:
>> Add an `indexOf()` variant allowing to specify both a lower and an upper
>> bound on the search.
>
> Raffaello Giulietti has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex)
In concept, having APIs that search a string subrange is a fine idea.
Unfortunately the exception handling policy is an issue. We need to think
through this carefully.
Currently, almost everything that takes some kind of String index or subrange
will throw IndexOutOfBoundsException if the arguments are ill-specified in some
fashion. There are a few notable outliers though:
indexOf(int ch, int fromIndex)
indexOf(String str, int fromIndex)
lastIndexOf(int ch, int fromIndex)
lastIndexOf(String str, int fromIndex)
regionMatches(boolean ignoreCase, int toffset, String other, int ooffset,
int len)
regionMatches(int toffset, String other, int ooffset, int len)
They don't throw any exceptions for ill-defined values; instead, they return -1
or false which is indistinguishable from "not found". These APIs date all the
way back to 1.0. Note that the JDK 1.0 String wasn't internally consistent. For
example, other 1.0-era methods like `substring` throw
StringIndexOutOfBoundsException.
The prevailing API style since that time has been to check arguments and throw
the appropriate exceptions, instead of masking ill-defined values by returning
"not found". This tends to reveal errors earlier instead of covering them up.
Compared to the existing `indexOf` methods (which specify a single starting
index), the new `indexOf` method specifies a subrange; this allows a new class
of "end < start" errors. It seems strange not to throw any exception for this
additional class of errors.
In understand the desire to be consistent with the existing methods. Adding a
new non-throwing method seems like a mistake though. It's perpetuating an old
API design mistake for the sake of consistency, while also being inconsistent
with current API design style.
I also don't think it's necessary to have both throwing and non-throwing
methods.
I'd suggest returning to the original `indexOf(ch, from, to)` proposal, but
instead having it check its subrange arguments and throwing
IndexOutOfBoundsException. I don't think the exception handling inconsistency
with the existing methods is that bad. If people really, really object to it,
then maybe it would be necessary to choose a new name for the new method (and
not introduce two versions). But choosing a good name is hard, and it
introduces issues such as discoverability and questions about how the new thing
differs from the existing methods, so I'm skeptical that choosing a different
name would be any better.
Another possible mitigation is to add API notes to highlight the unusual
behavior of the old non-throwing methods. Some of these old methods don't
mention their handling of illegal index values at all. (This could be done as
part of a separate PR.)
-------------
PR: https://git.openjdk.org/jdk/pull/12600
