Nomenclature cleanup discussion

[MooglyGuy]

Currently, MAME is very inconsistent in the naming scheme for read/write handlers, devcb read/write binders, and other things. I'd like to clean all of it up, and have the bandwidth to do so over the next week and a half, but I need some agreement on where to go with it.

I'd like to quickly settle on a consistent naming scheme here, as I've noticed the following things:

• The intended binding direction of a function is not always evident in its name.
• For writers, devices use write_*, some use *_out, some use *_out_cb, some use *_cb, some use just *, some use *_write, and the vast majority of cases use *_w.

There have been some valid concerns brought up about having prefixes/suffixes at all, given that the intent should be obvious based on a combination of usage and function signature, but the existing inconsistency is still harmful for potential new contributors.

I would like to propose the following:

• Binders in devices should follow a consistent naming scheme: NAME_r_cb and NAME_w_cb.
• Handlers such as write_*_cb, write_*, *_write, and so on will be replaced with *_w.
• Handlers that follow a more custom naming scheme (e.g. in/out, no specific direction) will be left as-is, as the intent can potentially be inferred from its usage or its function signature. However, I would prefer to eventually bring those over as well, but I need buy-in.

A question that immediately springs to mind is, how should binders in devices be named? Should it be from the perspective of the caller, or from the perspective of the device itself? If I'm providing a handler for a devcb_write_line foo, should I call the binder foo_r_cb or foo_w_cb?

In addition, I've noticed that VBL handlers for screen_device contain a myriad of creatively inconsistent names, including:

• screen_vblank
• vblank_irq
• vblank_int
• vblank_callback
• vblank_assert
• vblank_interrupt
• crtc_vsync
• crtc_vs
• *_vblank_irq
• vblank
• *_vsync_changed
• *_vsync_irq*

I would like to settle on a consistent naming scheme for these callbacks, but I'm open to suggestions.

[cuavas]

@galibert or @rb6502 do you have an opinion on this?

Personally, I’m happy the proposed convention of devcb_blah m_foo; being bound with foo_cb(), and turning read_foo/write_foo into the more common foo_r/foo_w in public device interfaces.

[MooglyGuy]

foo_cb, or foo_r_cb / foo_w_cb?

[cuavas]

Well, it depends on whether the _r/_w would actually add value:

• They’re necessary for devices that support pushed and pulled I/O (e.g. the 68705 family has both _r and _w callbacks for each GPIO port).
• They aren’t necessary for the callbacks on rs232_port_device because all the callbacks on the port are for incoming signals from the attached device (the direction of signals is defined in the CCIT V.24 standard as well).
• The adc0808_device currently has eoc_callback(), eoc_ff_callback() and in_callback() – I think it’s adequately clear that end of conversion (EOC) is going to be an output and in is going to be an input.
• There are a few bus devices with bidirectional signal lines that use _in on a handler and _out on a callback for the same line. I think these would be more confusing if they were both changed to _w to reflect the type rather than reflecting the signal drive direction.

There are a lot of cases like the RS232 port, where callbacks are only used for outputs. There are also a fair few devices like the ADCs where the direction can be inferred easily from the name.

It’s one of those things where I think trying to make a hard-and-fast rule that callbacks must or must not have the _r/_w qualifiers would do more harm than good. But in cases where there are matching read and write callbacks, or where tagging them as read and write makes sense, then _r/_w should be used for consistency rather than using _read/_write or some prefix scheme.

[MooglyGuy]

As observed when we were discussing this privately, your second point comes from a pre-existing familiarity with the RS-232 spec, and in a similar vein, your third point is equally reliant on existing familiarity with how ADCs work.

To be clear, I'm not shooting for a spec that is 100% applicable to all hardware configurations all of the time. What I want is some sort of cohesive naming scheme that can make the MAME codebase more approachable - and less likely to result in learning landmines - to someone who has never used it before.

To that end, if someone want to use rs232_port_device as a template for their implementation of some other serial device, I wouldn't necessarily expect them to know about CCIT V.24. Similarly, if someone wants to implement an ADC, I wouldn't necessarily expect them to know the details of how the ADC0808 works. What I would expect is for the code to implicitly give good guidance on how to implement their own serial device, or their own ADC.

I think there's room in the project for both goals - accurately documenting devices, and providing good guidance to new contributors pending code cleanup that I'll be doing - but there needs to be some concrete agreement of where the boundaries lie, and exactly how things should be stated, standards-wise.

I'm alright with things that deviate from those standards, but I want to at least bring the existing codebase up to a minimum quality point. If developers want to deviate from those norms from that point on, cool, but MAME still carries with it decades of poor-quality legacy code from when the project had far more of a Wild West vibe.

[rb6502]

One of the things that annoys me in MAME is when I need to use a device, I've read a datasheet to understand what's going on, and none of the device's public interfaces match the naming in the documentation for the device. So to an extent I think it's fine for RS232 to use EIA/CCIT naming and ADC0808 to use the signal names from the datasheet.

That said, I do think we can value-add on top of the manufacturer's naming with _r and _w or read_ and write_ or whatever.

[galibert]

I concur with Vas. It would clean things up nicely. And not having _w/_r on _cb allows not to have to think about which one to use :-)

[MooglyGuy]

Can you give an argument for why someone shouldn't think of which one to use?

I'm over here arguing that Vas should dial back the permissiveness of the devcb binder. A u8 handler should objectively not be allowed to compile as a function bound to a write_line handler, but right now, it does.

At the moment it seems like there's a level of permissiveness in terms of shoddy code quality which is allowed from all sides because the alternative is not just to require a more forceful push-back on shoddy pull requests, but an accounting for those which have been permitted as it is.

I hope that the level of permissiveness that's existed to this point is simply because finding people who actually want to engage in code cleanup is rare. I completely understand how things have gotten to this point. Fortunately, I'm here. :)

[rb6502]

I feel this is unnecessary overreach in its entirety, but I wouldn't object strongly to some kind of consistency on binders. Binders at least conceptually belong to the core. But renaming the actual callbacks people attach to the binders in their drivers is a fork-the-project level offense. Those names belong to the authors of those drivers.

Can we start with getting rid of the READ_LINE/WRITE_LINE macros though? That's unambiguously an improvement as long as you don't rename everyone's functions.

[rb6502]

Regarding what binders should look like, the convention I find most natural is read_foo bound by foo_r_callback() or foo_r_cb(), and the same with write_foo and foo_w_callback()/foo_w_cb().

From inside the device you use the very natural read_codec()/write_codec(). The m_ prefix is unnecessary because they're functions you call, not member variables. From outside I go back and forth on if the _r and _w should be flipped to indicate the device consumer's flow direction, but in general I have to side with them being the device's flow direction instead.

[MooglyGuy]

I think conflating "making the codebase more consistently approachable to new contributors" and "overreach" is itself a bit of a reach.

I'm not bringing this up because I want to castigate anyone, nor because I want to make anyone's life harder. When I was poring over the codebase, quite a number of my own drivers and devices came onto my radar as not being consistent with what I perceive to be the overall style of the codebase.

I think having read_* versus *_r as a differentiation between external/internal handlers (e.g. functions passed to binders vs. functions within a driver) is actually a pretty good idea, it's something that didn't occur to me but which makes sense.

All I really want is some agreed-upon bulleted list of guidelines that I can run through across the codebase. In discussing it with @galibert, it seems like there's an acknowledgement that the codebase needs some higher level of consistency than it currently has, but those hopes were written off due to tedium. I actually like doing those sorts of broad cleanups, so I'm volunteering to do it. I just need a spec to operate against.

[rb6502]

MAME's historically been attractive to contributors in part because we don't force things on people that are personal preference, like GNU vs K&R vs Allman bracing. (Or OG's insistence that the m_ prefix on member variables is terrible). I just don't want us to go too far homogenizing everything.

So my spec:

• binders: the callable from inside the device is read_ or write_, the bind function is _r_cb or _w_cb
• Get rid of the READ_LINE/WRITE_LINE macros, bare functions everywhere like read/write handlers and address maps
• Maybe change read_line/write_line to bool instead of int, but I can see that being an entire ordeal by itself

At some future point I'd be interested in getting rid of the ROM loading macros in favor of explicitly written out code, too. Similar to how address maps evolved. But that's a really big job, probably on the scale of "don't even think it until we decide to do another 2 month release".

[rb6502]

From your POV they're variables with a call operator because you're on both sides of the core/userland line and you know how they're implemented. But even if you accept knowing that they're actually fancy function pointers, m_whatever_w() or m_write_whatever() is still really clunky to read and write.

[MooglyGuy]

I'm not sure where the clunkiness comes in, practically every device I've seen still has the m_ prefix, since devcb_write_line is still a member of the class. For a practical example, I would advocate for:

class contoso_device : public device_t
{
public:
	contoso_device(const machine_config &mconfig, const char *tag, device_t *owner, u32 clock = 0);

	auto foo_r_cb() { return m_foo_r.bind(); }
	auto bar_w_cb() { return m_bar_w.bind(); }

	u8 regs_r(offs_t offset);
	void regs_w(offs_t offset, u8 data);

	u32 baz() { return m_baz; }

protected:
	virtual void device_start() override;
	virtual void device_reset() override;

	devcb_read_line m_foo_r;
	devcb_write_line m_foo_w;

	u32 m_baz;
};

...

class lipsum_state : public driver_device
{
public:
	lipsum_state(const machine_config &mconfig, device_type type, const char *tag) :
		driver_device(mconfig, type, tag)
	{
	}

	void lipsum(machine_config &config);

private:
	virtual void machine_start() override;
	virtual void machine_reset() override;

	void lipsum_map(address_map &map);

	int contoso_foo_r();
	void contoso_bar_w(int state);

	required_device<contoso_device> m_contoso;
};

...

void lipsum_state::lipsum(machine_config &config)
{
	CONTOSO(config, m_contoso);
	m_contoso->foo_r_cb().set(FUNC(lipsum_state::contoso_foo_r));
	m_contoso->bar_w_cb().set(FUNC(lipsum_state::contoso_bar_w));
}

[rb6502]

The clunk comes in two ways:

1. Having a single-character prefix and underscore on a function call just looks weird.
2. Once you have two decorators on something (m_ and _w) you're well on the way to reinventing Hungarian notation, but in a more intrusive way.

It's become increasingly clear that nobody gives a shit what I think because I'm not on fucking Discord though, so go off.

[MooglyGuy]

I can agree with your first point (which would make the second point irrelevant) if we can agree that devcb_* members tend to have a distinctly different usage pattern than most class members, and so should have a logically separate grouping of lines in the class declaration similar to how required_* and optional_* members are encouraged to be grouped separately.

As for not being on Discord, go to a bondage club with some good cuffs if you want to put yourself up on a cross, but spare the rest of us your self-victimization, because it doesn't help anyone. I'm not on the MAME Discord server, I communicate* with him 1:1. In fact, that's how I communicate with Vas, but through an entirely different program, because he's not even available for direct messaging on Discord the vast majority of the time. So in the wise words of Sgt. Hulka: Lighten up, Francis.

[rb6502]

I 100% agree that devcb members should be grouped together, that's generally the style I've moved towards.

[MooglyGuy]

The first two points are solid and sound good to me. The third is one of those hopeless things and is part of my justification for ditching *_LINE_MEMBER in the first place - the horse is far outside the barn at this point, and there's no real going back.

As for the point of "why MAME has been attractive to contributors", I'd pretty much just direct your attention to that classic image of a plane with a bunch of bullet-holes in it. The contributors that currently contribute to MAME have, for some, been drawn in by that freedom to forge their own path stylistically, that much is true.

But all the same, there have been people who are explicitly put off by virtue of wanting to learn the codebase, but have their hands full juggling the various conflicting paradigms that exist within the codebase. And I'm not going to pull a Haze, either, in terms of unnamed "them": I've spent quite a lot of time hand-holding this one particular "agg23" guy through the codebase as he's tried to glean enough info to make a MiSTer core for some Sharp SM5x-based LCD handhelds. I get the distinct impression that if I hadn't popped into a particular Discord server at the right time, he wouldn't have cottoned onto MAME at all.

And, not to put too fine a point on it, I seem to recall that you were one of the detractors of the MCFG_ removals back in the day. The same arguments that were made then could be made now: MAME provides an abstracted view of coding and that encourages contributors, look at how many people pop up to contribute each month, yadda yadda yadda. But you're also smart enough to recognize in retrospect that the number of contributors has continued to increase, most likely assisted in part by the shift towards more standardized C++ code.

I'm not arguing for complete homogeneity in the codebase, because that absolutely would be a buzzkill for contributors. All I'm asking for is avoiding the completely insane situation we have now, where I can literally list off 12 different ways of naming a fricking callback for screen_device's vertical-blank binder.

[galibert]

Ok. What we seem to agree on: _w/_r for memory handlers.

What we don't agree on: wtf to do with devcb, either line or not. A non-negligible part of the problem is what MG already identified, the directional semantics.

To get an example, let's see the nn71003f spi interface, it's charcteristic:
// Slave SPI interface
void ss_w(int state);
void sclk_w(int state);
void mosi_w(int state);
auto miso_cb() { return m_miso.bind(); }
devcb_write_line m_miso;

Forgetting my dislike for dual suffixes for a second, what should it be for the miso devcb? miso_w_cb() ? But it's in the opposite direction from the three other signals. miso_r_cb()? But it's a write line. miso_o_cb() (output)? That would be a new thing, and Vas could reasonably argue that "Master In Slave Out" already says it's output to anyone with a clue about spi.

Honestly, not sure what would be the best there.

[rb6502]

For callbacks, I think it works to have them from the device's POV, so write_miso() is the device writing to the callback and read_mosi() is the device reading from the callback. And for public member functions, it makes the most sense to have them from the device user's POV, so foo_r() reads from the device, and foo_w() writes to the device.

The reason this works is because in most cases you are connecting device-writes callbacks to user-writes functions, so you'd bind write_miso() to a theoretical spi_master_device::miso_w(). The write/w match. And similarly, you'd connect device-reads callbacks to user-reads, so read_mosi() would bind to spi_master_device::mosi_r(). The read/r match.

[galibert]

So your proposal is:

• *_r = direct read from the device
• *_w = direct write to the device
• write_* = devcb callback the device is going to write to
• read_* = devcb callback the device is going to write from

All that with no _cb in sight. Am I correct? Honestly, I can live with that easily. MG, Vas, opinions?

[galibert]

to read from obviously, duh

[MooglyGuy]

I'd like a _cb on the binder function itself, but I'm not married to it.

My personal preference would be write_* and read_* for lines specifically, rather than devcb specifically: My reasoning here is that the function signature for a memory-handler function is (mostly?) identical to the function signature for a function that can be bound to something like a devcb_write16 or devcb_write32. The swap from _r and _w to read_ and write_ is a subtle but clear indicator that the signature is going to either return int or take an int respectively, rather than potentially take offset, mem_mask, and either return or take data.

@rb6502 @galibert It sounds like we're pretty close to the same page at this point, but I'd really appreciate it if you guys would be willing to take a stab at putting the contoso/lipsum example into your ideal notation, just so I know where any differences still are. And for that matter, @cuavas, once you've taken some time to rest after the monthly release.

Personally, I don't care strongly one way or another what devcb_write* or devcb_read* member variables are named, the only thing I'm concerned with is the names of functions: The binders themselves, and the functions that get bound.

[galibert]

To add to the fun sync and blank are different things and iirc saturn can interrupt on either.

[rb6502]

Saturn also has a VDP1 drawing end interrupt, which optimistically is kind of a VBL interrupt but in reality VDP1 is rarely that fast :-)

[galibert]

Well, _r_cb and _w_cb are ugly as shit.

[rb6502]

They are ugly, but the problem of not understanding the data direction through something that's only named blah_callback is real.

[MooglyGuy]

This might just be a restatement of something further up in the thread, but I was talking to another dev who said he's always used _r and _w for memory handlers, and write_ and read_ for lines, and I have to admit, something about that idea does seem nice.

This would mean that the device function to bind a handler would be *_r/w_cb for memory and read/write_*_cb for lines, which doesn't solve the extremely constructive issue of them being "ugly as shit", but I tend to agree that indicating the data direction clearly from the binding function name is important, unless we want to ditch the "auto" return value from those functions, which I would hope is a non-starter.

[rb6502]

Yeah, I'm willing to accept ugly in this case for the better clarity. Definitely we're not getting rid of auto.

[rb6502]

Also, I like _r and _w for handlers and read_/write_ for lines. In part because that's very close to the style I've moved towards.

[cuavas]

You can’t get rid of auto return type on those functions anyway. They return private nested types that you can’t refer to by name to emphasise the fact that you shouldn’t be using them as anything but temporaries.