RFR: 8368775: Remove outdated comment in OutlineTextRenderer
Daniel Gredler
dgredler at openjdk.org
Thu Oct 2 10:00:56 UTC 2025
On Thu, 2 Oct 2025 07:14:56 GMT, Matthias Baesken <mbaesken at openjdk.org> wrote:
>>> > You can use str.isEmpty() here.
>>>
>>> I was actually going for consistency with all of the other optimizations of this type, which all use a length check. I can change it to `isEmpty` if you feel strongly about it, though.
>>
>> I think a lot of cases were written before isEmpty() existed so would have used length().
>> isEmpty() might perform a 'tiny' bit better. But that's all.
>>
>>>
>>> > Do we actually call this method with an empty string?
>>>
>>> I do think it's possible, e.g. from `SunGraphics2D.drawString(String, int, int)` and `SunGraphics2D.drawString(String, float, float)` when the font doesn't have layout attributes, or a few other places when `OutlineTextRenderer.drawChars(...)` delegates to `OutlineTextRenderer.drawString(...)`.
>>
>> I agree.
>> Even if we added some checks there, it would require work to prove that there's no way to get here with an empty string.
>> I see no harm in the check here, especially if we aren't 100% confident.
>
>> > You can use str.isEmpty() here.
>>
>> I was actually going for consistency with all of the other optimizations of this type, which all use a length check. I can change it to `isEmpty` if you feel strongly about it, though.
>>
>
> Hi, you replaced this strange `''".equals` check ; is this because it looks not nice compared to length or isEmpty or for other reasons ? I ask because there are quite a few of those checks in the codebase (below are only some of them) .
>
>
>
> src/java.base/share/classes/java/lang/runtime/ObjectMethods.java:416: List<String> nameList = "".equals(names) ? List.of() : List.of(names.split(";"));
> src/java.base/share/classes/java/net/NetworkInterface.java:230: return "".equals(displayName) ? null : displayName;
> src/java.base/share/classes/java/net/SocketPermission.java:885: if (this.wildcard && "".equals(this.cname))
> src/java.base/share/classes/java/security/CodeSource.java:466: if (("".equals(thisHost) || "localhost".equals(thisHost)) &&
> src/java.base/share/classes/java/security/CodeSource.java:467: ("".equals(thatHost) || "localhost".equals(thatHost))) {
> src/java.base/share/classes/java/text/CompactNumberFormat.java:2546: !"".equals(compactPatterns[index])) { // ignore empty pattern
> src/java.base/share/classes/sun/security/tools/keytool/Main.java:931: if ("".equals(dest)) {
> src/java.base/share/classes/sun/security/tools/keytool/Main.java:939: if ("".equals(alias)) {
> src/java.base/share/classes/sun/security/tools/keytool/Main.java:2510: if ("".equals(newAlias)) {
> src/java.base/share/classes/sun/security/util/SecurityProperties.java:155: if ("".equals(rawPropVal)) {
> src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java:604: } else if ("".equals(pattern)) {
> src/java.desktop/macosx/classes/com/apple/laf/AquaTextFieldSearch.java:246: if (!text.hasFocus() && "".equals(text.getText())) {
> src/java.desktop/macosx/classes/com/apple/laf/AquaTextFieldSearch.java:290: button.setVisible(!"".equals(text.getText()));
> src/java.desktop/share/classes/com/sun/media/sound/JDK13Services.java:175: if ("".equals(value)) {
> src/java.desktop/share/classes/java/awt/FileDialog.java:153: fileDialog.file = ("".equals(file)) ? null : file;
> src/java.desktop/share/classes/java/awt/FileDialog.java:156: fileDialog.dir = ("".equals(directory)) ? null : directory;
> src/java.des...
@MBaesken We decided to keep the check as a pure optimization (not just a safety check required to avoid an exception) after seeing that there are other similar checks (as optimizations) in the code base, and used this specific formulation simply for consistency with all of the other existing equivalent checks (see list here: https://github.com/openjdk/jdk/pull/26947#issuecomment-3329884998).
-------------
PR Comment: https://git.openjdk.org/jdk/pull/27523#issuecomment-3360249841
More information about the client-libs-dev
mailing list