On Wed, 6 Aug 2025 05:15:46 GMT, Shaojin Wen <s...@openjdk.org> wrote:
>> Jaikiran Pai has updated the pull request with a new target base due to a
>> merge or a rebase. The incremental webrev excludes the unrelated changes
>> brought in by the merge/rebase. The pull request contains four additional
>> commits since the last revision:
>>
>> - merge latest from master branch
>> - merge latest from master branch
>> - add test
>> - 8357708: com.sun.jndi.ldap.Connection ignores queued LDAP replies if
>> Connection is subsequently closed
>
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java line 87:
>
>> 85: ber.parseSeq(null);
>> 86: ber.parseInt();
>> 87: isLdapResResult = (ber.peekByte() ==
>> LdapClient.LDAP_REP_RESULT);
>
> Other boolean type variables in the LdapRequest class do not have the `is`
> prefix. The local variable `isLdapResResult` here should also use the same
> style. We should not use two styles in one class.
Calling it `ldapResResult` instead of `isLdapResResult` isn't appropriate here,
so I've let this stay in its current form.
> src/java.naming/share/classes/com/sun/jndi/ldap/LdapRequest.java line 147:
>
>> 145: }
>> 146: if (result == CANCELLED_MARKER) {
>> 147: throw new CommunicationException("Request: " + msgId +
>
> The current LdapRequest class should use a unified style to construct
> exception information, either using string concatenation or String.format. A
> class should not mix the two styles.
Hello Shaojin, these are pre-existing messages. I have however updated the PR
to use a uniform style.
> test/jdk/javax/naming/ldap/LdapClientConnTest.java line 110:
>
>> 108: try (final ExecutorService executor =
>> Executors.newCachedThreadPool()) {
>> 109: for (int i = 1; i <= numTasks; i++) {
>> 110: final String taskName = "task-" + i;
>
> Suggestion:
>
> String taskName = "task-" + i;
>
> Variables used only in the current block do not need to be final
This a more a personal preference and since this is a new test file, I let it
stay in this form. If there's a strong preference to remove it, I'll do so.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332405116
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332402149
PR Review Comment: https://git.openjdk.org/jdk/pull/25449#discussion_r2332413316
