heathkit/tlb.cpp: Various file cleanup #12275
Conversation
There was a problem hiding this comment.
Personally, I’m not a fan of tabulating initialisations, assignments and inline functions where it isn’t naturally tabular data involved. It either ends up ragged again quickly, or requires large numbers of lines to change for relatively small real changes.
I’m fine with tabulating stuff that’s naturally somewhat tabular, like I/O ports, ROM load commands, device type declarations and large blocks of variable declarations. It’s just that tabulating other things tends to lead to churn for minimal improvements in readability at best.
I’ll let other people take a look and chime in on this.
src/mame/heathkit/tlb.cpp
Outdated
| m_maincpu(*this, "maincpu"), | ||
| m_screen(*this, "screen"), | ||
| m_palette(*this, "palette"), | ||
| m_crtc(*this, "crtc"), | ||
| m_p_videoram(*this, "videoram"), | ||
| m_p_chargen(*this, "chargen"), | ||
| m_config(*this, "CONFIG"), | ||
| m_ace(*this, "ins8250"), | ||
| m_beep(*this, "beeper"), | ||
| m_mm5740(*this, "mm5740"), | ||
| m_kbdrom(*this, "keyboard"), | ||
| m_kbspecial(*this, "MODIFIERS"), | ||
| m_maincpu(*this, "maincpu"), | ||
| m_screen(*this, "screen"), | ||
| m_palette(*this, "palette"), | ||
| m_crtc(*this, "crtc"), | ||
| m_p_videoram(*this, "videoram"), | ||
| m_p_chargen(*this, "chargen"), | ||
| m_config(*this, "CONFIG"), | ||
| m_ace(*this, "ins8250"), | ||
| m_beep(*this, "beeper"), | ||
| m_mm5740(*this, "mm5740"), | ||
| m_kbdrom(*this, "keyboard"), | ||
| m_kbspecial(*this, "MODIFIERS"), | ||
| m_repeat_clock(*this, "repeatclock") |
There was a problem hiding this comment.
The trouble with tabulating everything like this is that if you introduce a data member with a longer name or remove the data member with the longest name, every line in the block needs to change.
Also, in this case you’ve tabulted the tags, but not the *this references.
There was a problem hiding this comment.
Ok, I undid these changes and the other ones you pointed out.
src/mame/heathkit/tlb.h
Outdated
| // optional operations | ||
| virtual void rlsd_in_w(int state) {} | ||
| virtual void dsr_in_w(int state) {} | ||
| virtual void cts_in_w(int state) {} | ||
| virtual void rlsd_in_w(int state) {} | ||
| virtual void dsr_in_w(int state) {} | ||
| virtual void cts_in_w(int state) {} | ||
|
|
||
| // optional SigmaSet operations | ||
| virtual void sigma_ctrl_w(uint8_t data) {} | ||
| virtual uint8_t sigma_ctrl_r() { return 0x00; } | ||
| virtual uint8_t sigma_video_mem_r() { return 0x00; } | ||
| virtual void sigma_video_mem_w(uint8_t val) {} | ||
| virtual void sigma_io_lo_addr_w(uint8_t val) {} | ||
| virtual void sigma_io_hi_addr_w(uint8_t val) {} | ||
| virtual void sigma_window_lo_addr_w(uint8_t val) {} | ||
| virtual void sigma_window_hi_addr_w(uint8_t val) {} | ||
| virtual void sigma_ctrl_w(u8 data) {} | ||
| virtual u8 sigma_ctrl_r() { return 0x00; } | ||
| virtual u8 sigma_video_mem_r() { return 0x00; } | ||
| virtual void sigma_video_mem_w(u8 val) {} | ||
| virtual void sigma_io_lo_addr_w(u8 val) {} | ||
| virtual void sigma_io_hi_addr_w(u8 val) {} | ||
| virtual void sigma_window_lo_addr_w(u8 val) {} | ||
| virtual void sigma_window_hi_addr_w(u8 val) {} |
There was a problem hiding this comment.
It’s the same with these – it’s prone to either ending up looking ragged again, or requiring all the lines in the block to change for a small functional change.
src/mame/heathkit/tlb.h
Outdated
| virtual const tiny_rom_entry *device_rom_region() const override; | ||
| virtual ioport_constructor device_input_ports() const override; | ||
| virtual void device_add_mconfig(machine_config &config) override; | ||
|
|
||
| void mem_map(address_map &map); | ||
| void mem_map(address_map &map); |
There was a problem hiding this comment.
See here’s an example where you’ve arbitrarily lined up device_add_mconfig and mem_map, but you haven’t lined up device_rom_region and device_input_ports with them. It’s a bit concerning if even the initial tabulation is inconsistent.
General cleanup including