On Wed, 6 Sep 2023 15:03:24 GMT, Adam Sotona <asot...@openjdk.org> wrote:
>> This PR improved Classfile API attributes handling safety by introduction of
>> attribute stability indicator
>> and by extension of UnknownAttributesOption to more general
>> AttributesProcessingOption.
>
> Adam Sotona has updated the pull request incrementally with one additional
> commit since the last revision:
>
> magic moved to Util::isAttributeAllowed and fixed possible NPE
Marked as reviewed by briangoetz (Reviewer).
src/java.base/share/classes/jdk/internal/classfile/AttributeMapper.java line
117:
> 115: * {@return attribute stability indicator}
> 116: */
> 117: AttributeStability attributeStability();
Perhaps the name `stability()` would be better here because all the methods
here are about the attribute being described.
src/java.base/share/classes/jdk/internal/classfile/impl/AbstractDirectBuilder.java
line 58:
> 56: attributes.withAttribute(a);
> 57: }
> 58: }
The "is allowed" check should always say yes for unbound attributes, and only
apply the option for bound attributes. (You may already have done that, but I
wanted to make sure this was captured in the review.)
src/java.base/share/classes/jdk/internal/classfile/impl/Util.java line 64:
> 62: ? ATTRIBUTE_STABILITY_COUNT -
> attr.attributeMapper().attributeStability().ordinal() >
> processingOption.ordinal()
> 63: : true;
> 64: }
Ignore earlier comment, looks like you've got this :)
-------------
PR Review: https://git.openjdk.org/jdk/pull/15101#pullrequestreview-1627325194
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326230242
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326233656
PR Review Comment: https://git.openjdk.org/jdk/pull/15101#discussion_r1326235687
