On Thu, 7 Nov 2024 21:21:57 GMT, Phil Race <[email protected]> wrote:
>> Jeremy has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - 8342782: Removing confusing javadoc
>>
>> This is in response to this PR comment:
>> https://github.com/openjdk/jdk/pull/21962#discussion_r1833401410
>> - 8342782: adding curly brackets
>>
>> This is in response to these PR comments:
>> https://github.com/openjdk/jdk/pull/21962#discussion_r1833399928
>> https://github.com/openjdk/jdk/pull/21962#discussion_r1833413838
>> - 8342782: removing new comment in javadoc
>>
>> This is in response to this PR comment:
>> https://github.com/openjdk/jdk/pull/21962#pullrequestreview-2422181991
>
> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 960:
>
>> 958: * Occasionally (after approximately 500 invocations) the root
>> 959: * AWTEventMulticaster is rebalanced.
>> 960: *
>
> I am not sure this comment is needed. If you think it is needed then it would
> need a CSR, and the comment should be tagged @implNote
I removed that comment.
> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 985:
>
>> 983: * <p>
>> 984: * The first time this method is invoked it will convert a tree
>> that has a degree
>> 985: * of approximately 500 to a tree with a degree of approximately 10.
>
> where does it do that ? I just see it deciding if there's a need to rebalance.
I removed this sentence. It confusingly conflated this method with the method
that actually did the rebalancing.
> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 991:
>
>> 989: while (true) {
>> 990: if (++level > 500)
>> 991: return true;
>
> our coding standards require that you always include the body in { .. }
@prrace : I added { .. }
@bourgesl : I'm struggling to define this as a constant in a helpful way. Is
there an example of a similar constant somewhere in the codebase you could
refer me to for comparison?
I think part of my challenge here is the AWTEventMulticaster doesn't promise to
be any particular kind of tree. (For example: if it promised it was a
self-balancing tree, then we'd know what our obligations were to meet that
expectation.)
This PR's implementation is intended to be a fast heuristic/educated guess, but
it does not scan the entire height of the left or right node. This makes it ...
hard for me to name.
(Relatedly: the value 500 is arbitrary, and I'm happy to change it if someone
would rather it be closer to 50 or 1000.)
> src/java.desktop/share/classes/java/awt/AWTEventMulticaster.java line 1018:
>
>> 1016: */
>> 1017: private static EventListener rebalance(EventListener[] array, int
>> index0, int index1) {
>> 1018: if (index0 == index1)
>
> { .. } again
I added { .. }
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390611
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390855
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390839
PR Review Comment: https://git.openjdk.org/jdk/pull/21962#discussion_r1855390868
