On Tue, 14 May 2024 08:30:56 GMT, Aggelos Biboudis <[email protected]>
wrote:
>> Adam Sotona has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - fixed error thrown by VerifierImpl
>> - applied suggested changes
>
> src/java.base/share/classes/jdk/internal/classfile/impl/verifier/ParserVerifier.java
> line 223:
>
>> 221: errors.add(new VerifyError("Arguments can't fit
>> into locals in %s".formatted(toString(ae))));
>> 222: }
>> 223: int l = 12 + ca.codeLength() + 8 *
>> ca.exceptionHandlers().size();
>
> nit (or not): it would be amazing if all these constants have a name. They
> are almost intuitive, I admit, but it would improve readability a lot.
I think about the readability exactly opposite. When deciding between:
static final int CODE_ATTRIBUTE_HEADER_SIZE = 12;
static final int CODE_ATTRIBUTE_EXCEPTION_HANDLER_ENTRY_SIZE = 8;
...
int l = CODE_ATTRIBUTE_HEADER_SIZE + ca.codeLength() +
CODE_ATTRIBUTE_EXCEPTION_HANDLER_ENTRY_SIZE * ca.exceptionHandlers().size();
or
int l = 12 + ca.codeLength() + 8 * ca.exceptionHandlers().size();
For this specific case ( implementation transcription of `classFileParser.cpp`)
I prefer the shorter.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/16809#discussion_r1599666623
