cellCamera: Improvements and refactoring #10818
Conversation
|
Wait with merging, I need some more cleanup in cellCamera. |
elad335
changed the title
elad335
force-pushed
the
camera-fixup
branch
from
2bdb57d
to
4a6ca3a
Compare
elad335
changed the title
|
|
|
||
| switch (state) | ||
| { | ||
| case camera_state::detached: return not_an_error(CELL_CAMERA_ERROR_DEVICE_NOT_FOUND); |
There was a problem hiding this comment.
Why is is this notanerror
There was a problem hiding this comment.
Because it's a null handler error.
elad335
force-pushed
the
camera-fixup
branch
3 times, most recently
from
8fc2d30
to
469e6cd
Compare
| } | ||
|
|
||
| if (func()) | ||
| { |
There was a problem hiding this comment.
I'm not a fan of what you're doing here tbh.
- It never reaches this point if the camera state is correct. So it is only checked if the camera state is wrong anyway.
- It is kinda spaghetti code and hard to read in the flow of the functions
- It adds another abstraction that you need to wrap your head around if you try to match assembly to code for example
So I personally would prefer if this function stayed a state check, and the param checks would remain conventional.
There was a problem hiding this comment.
Oh, I see the ordering bug, will fix.
Although it packs common logic which in return reduces amount of LOC significantly. IMO it should be better documented as an optional ""optimization"" for code size. When this optimization cannot be followed one shall return to the manual coding of checks.
There was a problem hiding this comment.
I appreciate the attempt to reduce LOC.
But i think in this case it's not really necessary, especially since it meddles with history as well
There was a problem hiding this comment.
And it makes this PR harder to review as well 💩
There was a problem hiding this comment.
The main reason I'm modifying it in the first place is to remove unneeded variables for savestates (better to remove before PR is merged), history will be modified either way.
Everything in addition is a bonus.
There was a problem hiding this comment.
Hmm maybe I can unwrap it relatively easily.
elad335
force-pushed
the
camera-fixup
branch
from
469e6cd
to
6510898
Compare
|
Fixed review comments. |
elad335
force-pushed
the
camera-fixup
branch
from
6510898
to
32e8cd3
Compare
* Fixed cellCameraEnd memleak. * Fixed many error checks of null handler. * Fixed cellCameraGetBufferSize. (override width/height, not bytesize) * Fixed cellCameraIsAttached, remove hacks! * Fixed cellCameraOpenEx. (check version!) * Fixed cellCameraIsAvailable. * Added nullptr check in cellCameraGetBufferInfo. * Simplify common error checking. * Simplify and atomic-ize cellCamera state management.
elad335
force-pushed
the
camera-fixup
branch
from
32e8cd3
to
0ca235b
Compare
|
Can you remove the check_errors function for now? |
|
This is now very outdated |
|
I don't even remember what it does, rip. |
This pull request is also needed for savestates.