search

beardypig / ghidra-emotionengine

Ghidra Processor for the Play Station 2's Emotion Engine MIPS based CPU
Apache License 2.0
200 stars 34 forks source link

InjectPayloadVu Tests and Refactor #31

Closed astrelsky closed 3 years ago

astrelsky commented 4 years ago

I'm not sure why I did it the way I originally did. My only guess was that I had gotten so used to pythons f-string that it hadn't occurred to me to check if Java had string formatting.

astrelsky commented 4 years ago

Looks good. Some formatting issues (mixed tabs/spaces), but other than that it's now much easier to read.

I'm not entirely convinced extracting all the format strings as separate constants is beneficial, as I find it useful to have that information inline so I can see how the arguments get mapped to characters, and most are only used once. I suppose that's just a matter of taste.

I'll go back and fix the mixed tabs/spaces as I didn't realize it. It probably occurred while regexing some things. As for the constant format strings, from experience with other languages such as c++ I'm under the impression that constructing strings should be limited as much as possible because they allocate storage on the heap. I'll admit that I don't know how they work in Java though. I will definitely go back and put in some comments with the value of the strings where they are used though.

beardypig said he is going to get a proper testing setup. I hacked the test part together and will admit I have no idea what I'm doing there.

chaoticgd commented 4 years ago

I'm under the impression that constructing strings should be limited as much as possible because they allocate storage on the heap

If you're just passing a string literal to a function, I don't think it should be a problem. If you're concatenating string literals, a decent compiler should be able to work everything out at compile time.

astrelsky commented 4 years ago

I'm under the impression that constructing strings should be limited as much as possible because they allocate storage on the heap

If you're just passing a string literal to a function, I don't think it should be a problem. If you're concatenating string literals, a decent compiler should be able to work everything out at compile time.

I stepped through it with a debugger to check, which confirmed you are correct, and noticed that I may actually be able to construct a constant formatter and use it's StringBuilder instead of reconstructing those each time. It may just be needless optimization though. I do know that the decompiler invokes these methods each time you switch to a function containing a vu instruction so these are being executed potentially thousands of times.

I'll probably inline the format strings which are only used once and leave the constants for once which are used multiple times.

astrelsky commented 4 years ago

So the tab/space mixing was due to my own negligence. I've converted the indentations to a tab character the size of 4 spaces which is what the Ghidra Java Coding Standards says to use.

I've also placed some short strings that were only being used once directly into where they are being used.

beardypig commented 3 years ago

Superseded by #41.