Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Address some limitations of delegates and callbacks #11333

Merged
merged 7 commits into from Jun 16, 2023

Conversation

cuavas
Copy link
Member

@cuavas cuavas commented Jun 12, 2023

This is mostly a response to comments from @rb6502 and @galibert. This is split up into three commits to make it a bit easier to see what’s going on:

  • The actual functional changes in src/emu (not possible to build as it lacks the corresponding updates to the rest of the code)
  • Updating the rest of the code for the changes to devcb.h
  • Hacks to make three devices I haven’t sorted out yet build

The big change answers the question, “Why does devcb have resolve as well as resolve_safe?” by making removing them both altogether. This entails a number of changes:

  • Device callbacks are now automatically resolved at the same time as object finders.
  • Read callbacks now need a default return value specified at construction, whether you plan to use it or not.
  • The isnull() accessor and cast to bool operator have been removed.
  • There’s a new isunset() accessor that tells you if the callback wasn’t configured, even when it’s callable after being resolved.

I can think of arguments against this:

  • You no longer have to make a conscious decision on whether you’re going to do things differently depending on whether the callback is configured or not. Currently, if you use resolve you need to check that the callback is configured every time. With this change you play fast and loose.
  • You always need to supply a default return value for read callbacks, even if you’re going to check them with isunset every time and do something different if they aren’t configured.

This does make the code tidier, though:

  • One less thing to remember to do when using device callbacks
  • Many devices no longer need to implement device_resolve_objects
  • Over 6,000 lines net reduction

@cuavas cuavas requested review from galibert and rb6502 June 12, 2023 14:30
@rb6502
Copy link
Contributor

rb6502 commented Jun 12, 2023

Now it would be possible to go a step further with this and remove the need for resolve on callbacks altogether, but that would mean moving the default return value to the constructor arguments rather than setting it in device_start. However, it would reduce total line count and avoid possible confusion between callback and device delegate resolve semantics.

I like this idea a lot. Combine it with requiring the default value to be explicitly set (I also don't like the default zero) and I think it's a winner.

@cuavas
Copy link
Member Author

cuavas commented Jun 13, 2023

I’ll separate the device delegate part since that isn’t controversial anyway. Then I can mess around with getting rid of resolve on callbacks. (Device delegates will still have resolve, but callbacks won’t need it.)

@galibert
Copy link
Member

I don't think there's anything controversial in there, only cool stuff.

One thing I would find nice, if you go and tweak all devdb constructors, is to add a name to them. Multiple times I would have liked a gtkwave-like display of live devcbs in the debugger (for instance to debug adb) but without naming it's impractical.

@cuavas
Copy link
Member Author

cuavas commented Jun 13, 2023

Sorry, named devcbs aren’t going to make it this time. I’ll put it on my TODO list.

@galibert
Copy link
Member

No worries :-)

@rb6502
Copy link
Contributor

rb6502 commented Jun 14, 2023

I love the named devcbs idea, but it's fine to wait on that too.

Read callbacks now need a default return value supplied at constrcution.

Replaced isnull() with isunset() which tells you if the callback wasn't
configured rather than whether it isn't safe to call.

Enabled validation of device callbacks (it seems it was disabled at some
point, probably accidentally).

Device callbacks and object finders now implement the same interface for
resolution.
@cuavas cuavas merged commit e9c1f4a into mamedev:master Jun 16, 2023
1 of 5 checks passed
@cuavas cuavas deleted the emubuster branch June 16, 2023 15:10
pauldevine pushed a commit to pauldevine/mame that referenced this pull request Jun 21, 2023
…edev#11333)

Read callbacks now need a default return value supplied at construction.

Replaced isnull() with isunset() which tells you if the callback wasn't
configured rather than whether it isn't safe to call.

Enabled validation of device callbacks (it seems it was disabled at some
point, probably accidentally).

Device callbacks and object finders now implement the same interface for
resolution.
sonninnos pushed a commit to libretro/mame that referenced this pull request Jul 20, 2023
…edev#11333)

Read callbacks now need a default return value supplied at construction.

Replaced isnull() with isunset() which tells you if the callback wasn't
configured rather than whether it isn't safe to call.

Enabled validation of device callbacks (it seems it was disabled at some
point, probably accidentally).

Device callbacks and object finders now implement the same interface for
resolution.
felipesanches added a commit to felipesanches/mame that referenced this pull request Dec 26, 2023
felipesanches added a commit to felipesanches/mame that referenced this pull request Jan 1, 2024
cuavas pushed a commit that referenced this pull request Jan 22, 2024
Fixes handlers not being called after e9c1f4a (GitHub PR #11333).
MooglyGuy pushed a commit to MooglyGuy/mame that referenced this pull request Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants