Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
On Thu, 23 Mar 2023 22:50:28 GMT, Jonathan Gibbons wrote: >> Hannes Greule has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Apply suggestions from code review >> >> Co-authored-by: liach <7806504+li...@users.noreply.github.com> > > test/jdk/jdk/classfile/BoundAttributeTest.java line 46: > >> 44: /* >> 45: * @test >> 46: * @issue 8304837 > > Should be `@bug`, not `@issue`. > > Did you run this test? Did it not complain at `@issue`? sorry, i apparently suggested a wrong tag on github. my bad Suggestion: * @bug 8304837 - PR Review Comment: https://git.openjdk.org/jdk/pull/13167#discussion_r1147030659
Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
On Thu, 23 Mar 2023 22:34:24 GMT, Hannes Greule wrote: >> After merging master into https://github.com/openjdk/jdk/pull/9862, we >> encountered test failures (e.g., >> https://github.com/SirYwell/jdk/actions/runs/4500940829/jobs/7923018438#step:9:2541). >> The Classfile API tries to read from constant pool index 0 if a >> MethodParameters attribute has an entry without name. >> >> The fix is simply using `readUtf8EntryOrNull` instead of `readUtf8Entry`. >> The related code already correctly handles nullability. >> >> I didn't find an appropriate test class so I added a new one. Let me know if >> there's a better place or if the test can be improved somehow. >> >> As I don't have a JBS account, someone needs to create a bug report there >> for me. Thanks. > > Hannes Greule has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> test/jdk/jdk/classfile/BoundAttributeTest.java line 44: > 42: import static org.junit.jupiter.api.Assertions.assertTrue; > 43: > 44: /* Generally, the style for tests is to put the test description comment _before_ the imports. - PR Review Comment: https://git.openjdk.org/jdk/pull/13167#discussion_r1146955092
Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
On Thu, 23 Mar 2023 22:34:24 GMT, Hannes Greule wrote: >> After merging master into https://github.com/openjdk/jdk/pull/9862, we >> encountered test failures (e.g., >> https://github.com/SirYwell/jdk/actions/runs/4500940829/jobs/7923018438#step:9:2541). >> The Classfile API tries to read from constant pool index 0 if a >> MethodParameters attribute has an entry without name. >> >> The fix is simply using `readUtf8EntryOrNull` instead of `readUtf8Entry`. >> The related code already correctly handles nullability. >> >> I didn't find an appropriate test class so I added a new one. Let me know if >> there's a better place or if the test can be improved somehow. >> >> As I don't have a JBS account, someone needs to create a bug report there >> for me. Thanks. > > Hannes Greule has updated the pull request incrementally with one additional > commit since the last revision: > > Apply suggestions from code review > > Co-authored-by: liach <7806504+li...@users.noreply.github.com> test/jdk/jdk/classfile/BoundAttributeTest.java line 46: > 44: /* > 45: * @test > 46: * @issue 8304837 Should be `@bug`, not `@issue`. Did you run this test? Did it not complain at `@issue`? - PR Review Comment: https://git.openjdk.org/jdk/pull/13167#discussion_r1146952634
Re: RFR: 8304837: Classfile API throws IOOBE for MethodParameters attribute without parameter names [v2]
> After merging master into https://github.com/openjdk/jdk/pull/9862, we > encountered test failures (e.g., > https://github.com/SirYwell/jdk/actions/runs/4500940829/jobs/7923018438#step:9:2541). > The Classfile API tries to read from constant pool index 0 if a > MethodParameters attribute has an entry without name. > > The fix is simply using `readUtf8EntryOrNull` instead of `readUtf8Entry`. The > related code already correctly handles nullability. > > I didn't find an appropriate test class so I added a new one. Let me know if > there's a better place or if the test can be improved somehow. > > As I don't have a JBS account, someone needs to create a bug report there for > me. Thanks. Hannes Greule has updated the pull request incrementally with one additional commit since the last revision: Apply suggestions from code review Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/13167/files - new: https://git.openjdk.org/jdk/pull/13167/files/4a94bb17..f4b8062c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=13167=01 - incr: https://webrevs.openjdk.org/?repo=jdk=13167=00-01 Stats: 2 lines in 2 files changed: 1 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/13167.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/13167/head:pull/13167 PR: https://git.openjdk.org/jdk/pull/13167
