garydgregory commented on PR #58: URL: https://github.com/apache/commons-bcel/pull/58#issuecomment-6563690429
@Hippo Thanks for working on BCEL-341. The historical compact Code header is a real compatibility issue, but I think the patch needs a few changes before it is ready to merge, so I created PR 542. Notes on this PR. - Preserve existing parsing behavior. Modern JVMs also accept versions 45.0 -> 45.2 with standard Code headers. Selecting the compact layout solely from the version breaks files BCEL could previously parse. An explicit parsing option, with the standard layout remaining the default, would preserve compatibility. - Cover code generation and version changes. The header setting needs to survive copying and regeneration through `MethodGen` and `BCELifier`. Dumping a class must also select a consistent layout, including when its version changes to 45.3 or later. - Correct the test fixture. The added fixture declares max_locals = 0 for `main(String[])`, which requires at least one local-variable slot for its argument. The fixture should be loadable by a JVM that supports its format. - Expand regression coverage. Besides parsing and dumping, tests should exercise standard headers with older version numbers, field-width limits, copying, code generation, and class-version changes. The Javadoc should also describe exactly which fields change width and how the layout is selected. Closing in favor of PR 542 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
