search

staktrace / cafebabe

A java class file parser
43 stars 12 forks source link

Make more `Opcodes` hold a `FieldType` #40

Closed C0D3-M4513R closed 2 months ago

C0D3-M4513R commented 3 months ago

I wanted to include a more exhaustive test, but that was prevented by #41

staktrace commented 3 months ago

I think this PR is fundamentally flawed because you're trying to use a FieldType out of convenience (i.e it represents approximately the things you care about) rather than out of correctness. If you need to have a more strongly typed operand here for the opcodes, I'd suggest creating a new type to represent that more exactly. Let me know if you disagree!

C0D3-M4513R commented 3 months ago

I think this is entirely justified. See the bytecode in #41. You have some types there that entirely match the FieldType.

staktrace commented 3 months ago

I think this is entirely justified.

Are you referring to your PR, or my response?

See the bytecode in #41. You have some types there that entirely match the FieldType.

But would those types (in the case where they are valid) be used for these opcodes? And even if there is some overlap, I wouldn't want to use the type unless it is semantically equivalent, which it doesn't seem like it is. In my experience reusing code for syntactic/structural reasons usually ends up getting a mistake. I'm happy to explain more in detail if you'd like what I mean by that.

C0D3-M4513R commented 3 months ago

https://github.com/Col-E/Recaf/issues/837#issuecomment-2267600952

staktrace commented 2 months ago

I implemented this to my satisfaction in https://github.com/staktrace/cafebabe/pull/49.