On Fri, 2 Aug 2024 07:57:27 GMT, Adam Sotona <asot...@openjdk.org> wrote:

>> Chen Liang 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 three additional 
>> commits since the last revision:
>> 
>>  - Remove generics, further improve docs
>>  - Merge branch 'master' of https://github.com/openjdk/jdk into 
>> fix/annotation-constant
>>  - 8335927: Revisit AnnotationConstantValueEntry and 
>> AnnotationValue.OfConstant
>
> src/java.base/share/classes/java/lang/classfile/AnnotationValue.java line 90:
> 
>> 88:      */
>> 89:     @PreviewFeature(feature = PreviewFeature.Feature.CLASSFILE_API)
>> 90:     sealed interface OfConstant<C extends AnnotationConstantValueEntry, 
>> R extends Comparable<R> & Constable>
> 
> I suggest to minimize use of generics in combination with sealed interface 
> trees and pattern matching. It does not give many benefits, it takes out a 
> part of the code readability and adds many unnecessary brackets and 
> questionmarks: 
> 
> case AnnotationValue.OfConstant<?, ?> oc ->
> 
> 
> `<R extends Comparable<R> & Constable>` at least brings a benefit of types 
> combination. However I still think it does not outweigh the generics use 
> cost. None of the generics parameter is near to orthogonal to the 
> `OfConstant` nor critical for the API to operate.

Removed the type parameters. Please review again.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/20436#discussion_r1701848599

Reply via email to