On Thu, 27 Mar 2025 14:56:17 GMT, Adam Sotona <asot...@openjdk.org> wrote:
> I'm sorry, but I'm still missing the benefits part of this change. The benefits are that the user can parse and generate class files directly from and to mapped files, and parse class files directly from buffers given to a class loader (for example). On the parsing side, there is an extra copy (this is not worse than today), but it is theoretically possible that we could optimize this someday. At absolute worst, it eliminates the need for the user to do this copy manually. On the generation side, there is no extra copy needed in comparison to the byte array implementation. > The costs are high. The API grows, implementation splits into multiple > parallel branches in ClassFileImpl and DirectClassBuilder. I'm not sure I follow you here. The implementation is only minimally changed. Half of the 419 lines added are javadocs. I'm not sure I understand where the new costs are coming from. > A new implementation class BuildAndParseBuffersAndSegments appears. This is just a test class. It proves the functionality of the new API in terms of the existing API; this means that it is not necessary to retest all functionality for both input types. > Parallel implementations add a new dimension to the test coverage matrix. It shouldn't; there is not a parallel implementation, and I think it's reasonable to at least make the argument that there should not be in the future either. Nothing here will force that issue one way or the other. As I said above, because there is still only one implementation, one test class is sufficient to show that parsing and generating to memory segments is working correctly. > There is also still a lot of code related to ByteBuffer. All the `ByteBuffer` code is dead and was intended to be deleted; that was an error on my part. I've fixed the patch now if you would like to take another look. > I'm not convinced the API should be extended with the MemorySegment overrides. I hope you reconsider in light of the above explanations. Thanks! ------------- PR Comment: https://git.openjdk.org/jdk/pull/24139#issuecomment-2758483091
