Skip to content

Conversation

ZoeB
Copy link
Contributor

@ZoeB ZoeB commented Apr 11, 2015

I'm purposefully leaving /src/emu/bus/cbmiec/c1541.c's kernal.bin
as it is, as this particular spelling mistake was originally made
by Robert Russell, therefore is canon. See
http://en.wikipedia.org/wiki/KERNAL for details.

I'm also leaving /src/emu/machine/nscsi_bus.c's RECIEVE as I don't
want to break anything, but it's worth someone looking into.

I renamed some variables in /src/mame/drivers/sfbonus.c,
/src/mame/video/tia.c and /src/mame/video/tia.h, so if anyone wants
to verify I didn't break anything, that would be nice.

I'm purposefully leaving /src/emu/bus/cbmiec/c1541.c's kernal.bin
as it is, as this particular spelling mistake was originally made
by Robert Russell, therefore is canon.  See
http://en.wikipedia.org/wiki/KERNAL for details.

I'm also leaving /src/emu/machine/nscsi_bus.c's RECIEVE as I don't
want to break anything, but it's worth someone looking into.

I renamed some variables in /src/mame/drivers/sfbonus.c,
/src/mame/video/tia.c and /src/mame/video/tia.h, so if anyone wants
to verify I didn't break anything, that would be nice.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third Millenium Engineering is a trademark and is spelled with a single 'n'. Please check properly before attempting to 'correct' the spelling of names/trademarks.

@balr0g
Copy link
Contributor

balr0g commented Apr 11, 2015

I don't know if I would call "publically" legitimate. Most authoritative sources consider it at least nonstandard, and many consider it wrong.

"Third Millenium" is correct as-is — see http://apple2info.net/index.php?title=The_Arcade_Board .

As for variable renames: I'm not sure how the developers other than @cuavas feel about this, but personally it really irks me if I'm working with code with such misspellings. And variable names change pretty-frequently as-is. Personally I'd rather have correctly spelled variable names over the temporary convenience of not having to refresh autocomplete data.

@ghost
Copy link

ghost commented Apr 11, 2015

I agree. unless there is a legitimate US/UK spelling difference, the
spelling of any dictionary word, including variable names should be correct.

Guru

On Sat, Apr 11, 2015 at 9:04 AM, balr0g notifications@github.com wrote:

I don't know if I would call "publically" legitimate. Most authoritative
sources consider it at least nonstandard, and many consider it wrong.

"Third Millenium" is correct as-is — see
http://apple2info.net/index.php?title=The_Arcade_Board .

As for variable renames: I'm not sure how the developers other than
@cuavas https://github.com/cuavas feel about this, but personally it
really irks me if I'm working with code with such misspellings. And
variable names change pretty-frequently as-is. Personally I'd rather have
correctly spelled variable names over the temporary convenience of not
having to refresh autocomplete data.


Reply to this email directly or view it on GitHub
#159 (comment).

@cuavas
Copy link
Member

cuavas commented Apr 11, 2015

Messing with variable and function names also has greater chance of causing conflicts when people are working on the code, and causing unintended side-effects if you unintentionally end up shadowing a global or member. I still don't think it's a good idea to just change variable names when you aren't doing any other work on the code, but if you're going to, at the very least do a build with -Wshadow and test affected drivers.

@ghost
Copy link

ghost commented Apr 11, 2015

if a full source-wide rename is done there shouldn't be issues since all
affected variables are renamed.
I seem to recall Aaron did this many times in the past. Sure he was working
on the code but it's still just a variable renamed and if all of them are
changed through the entire source codebase it functions exactly the same as
before the rename. only now it looks more professional because the spelling
is correct ;-)

On Sat, Apr 11, 2015 at 3:28 PM, Vas Crabb notifications@github.com wrote:

Messing with variable and function names also has greater chance of
causing conflicts when people are working on the code, and causing
unintended side-effects if you unintentionally end up shadowing a global or
member. I still don't think it's a good idea to just change variable names
when you aren't doing any other work on the code, but if you're going to,
at the very least do a build with -Wshadow and test affected drivers.


Reply to this email directly or view it on GitHub
#159 (comment).

@ZoeB
Copy link
Contributor Author

ZoeB commented Apr 11, 2015

Huh. They spelt Millennium correctly on the PCB: http://apple2info.net/images/thumb/3/3d/TME_ArcadeBoard.jpg/480px-TME_ArcadeBoard.jpg I guess they weren't too consistent. :) Reverting...

ZoeB added 3 commits April 11, 2015 10:24
Apparently this board is trademarked with the spelling mistake,
therefore let's keep the incorrect spelling.  Curiously, they spelt
Millennium correctly on the PCB, so the original seems a tad
inconsistent in this area.
Allow the variant spelling "publically".
@ZoeB
Copy link
Contributor Author

ZoeB commented Apr 11, 2015

I've reverted the Millennium fix, the publicly fix, and the variable fix, so you can pick and choose (or, rather, cherry-pick and squash) which of these you'd like and which you wouldn't.

@cuavas
Copy link
Member

cuavas commented Apr 11, 2015

@mr-t-guru a global rename can still cause a shadowing issue. Suppose there's a more global variable called "address" and a more local variable called "addres". The incorrectly spelled variable doesn't shadow the correctly spelled one. But if you rename "addres" to "address" you create a shadowing issue even if you catch all occurrences of it, as any code where the variable formerly known as "addres" is in scope that tries to access the more global variable will access the more local variable instead as they now have the same name.

@happppp
Copy link
Member

happppp commented Apr 11, 2015

all the bla aside, the "catagory" variable typo is ok to fix :|
The COLUM one: well, a quick look tells me there's M and P, COLUP and COLUM

On Sat, Apr 11, 2015 at 2:49 PM, Vas Crabb notifications@github.com wrote:

@mr-t-guru https://github.com/mr-t-guru a global rename can still cause
a shadowing issue. Suppose there's a more global variable called "address"
and a more local variable called "addres". The incorrectly spelled variable
doesn't shadow the correctly spelled one. But if you rename "addres" to
"address" you create a shadowing issue even if you catch all occurrences of
it, as any code where the variable formerly known as "addres" is in scope
that tries to access the more global variable will access the more local
variable instead as they now have the same name.


Reply to this email directly or view it on GitHub
#159 (comment).

@ZoeB
Copy link
Contributor Author

ZoeB commented Apr 11, 2015

Ack, you're right, COLUM should stay as it is! Nice catch, thanks. I'll make a version with "catagory" changed but COLUM left well alone, on the offchance that's a combination that would be more useful. Again, feel free to cherry pick and squash / rebase these as needed to only keep the good changes.

@ghost
Copy link

ghost commented Apr 11, 2015

the solution is to plan and be smarter from the start and to NOT use
keywords or commonly used computer terms as variables. If you want to use
some common computer-word like "address" as a variable you are just asking
for trouble. If you miss-spell it and continue to use the miss-spelled
version multiple times that is just lazy or stupid (or both).
hence the use of long obscure multi-word variables through-out MAME that
are easily mass-renamed without errors.
anyway I'm not going to argue with you about it. She should probably just
stick with non-code changes/corrections anyway because she doesn't have a
MAME-background or knowledge of the MAME-Way(tm)

On Sat, Apr 11, 2015 at 8:49 PM, Vas Crabb notifications@github.com wrote:

@mr-t-guru https://github.com/mr-t-guru a global rename can still cause
a shadowing issue. Suppose there's a more global variable called "address"
and a more local variable called "addres". The incorrectly spelled variable
doesn't shadow the correctly spelled one. But if you rename "addres" to
"address" you create a shadowing issue even if you catch all occurrences of
it, as any code where the variable formerly known as "addres" is in scope
that tries to access the more global variable will access the more local
variable instead as they now have the same name.


Reply to this email directly or view it on GitHub
#159 (comment).

@cuavas cuavas merged commit 373d338 into mamedev:master Apr 11, 2015
@cuavas
Copy link
Member

cuavas commented Apr 11, 2015

In an ideal world things no-one would repeat a misspelling, but it isn't an ideal world:

  • People use editors/IDEs with autocomplete, so when you're working at speed you misspell the name once in the declaration, then use autocomplete each time you use it, so you don't notice the misspelling because you don't have to type it out each time. This is far easier to do with long names than short ones due to clutter factor.
  • MAME code is in English and many MAME developers are non-native speakers. Many spelling errors are honest mistakes. These tend to be done consistently throughout a given developer's work. If you want all contributors to be fluent English speakers you're going to lose a lot of people.
  • Even with English speakers, you can end up with regional spelling differences causing equivalently named symbols to end up in scope, e.g. colour/color and centre/center.

Giving variables overly verbose names doesn't help. If you have a bus write handler, you're going to name the parameters something along the lines of address, data and mask. In fact, that's what MAME's core macros enforce: your parameters are called address_space, offset, data and mem_mask. Terse names are easy to understand and use.

Shadowing can be avoided by using conventions like prefixing member names with m_ and global names with g_, but some people on the team find this style distasteful. Ultimately we should be building with -Wshadow all the time - it would alert us to a lot of potential problems before they become real headaches. The trouble is, MAME has gone far too long without this, so it causes a huge number of warnings, and it will take a long time to go through and address them all before -Wshadow can be enabled by default.

@ghost
Copy link

ghost commented Apr 12, 2015

so it should be done. MAME can't get any worse ;-)
Maybe ZoeB can look into it and make a real improvement so we can eventually enable -Wshadow

ICEknigh7 added a commit to ICEknigh7/mame that referenced this pull request Jun 8, 2022
- Balalín (type-in, MicroHobby mamedev#125) [Ignacio Prini]
- Magnus Zone (type-in, MicroHobby mamedev#126) [Ignacio Prini]
- Oráculo Egipcio (type-in, MicroHobby mamedev#127) [Ignacio Prini]
- Demolition (type-in, MicroHobby mamedev#128) [Ignacio Prini]
- Basket Trainer (type-in, MicroHobby mamedev#129) [Ignacio Prini]
- Galaxy Fun (type-in, MicroHobby mamedev#130) [Ignacio Prini]
- Micro Draw (type-in, MicroHobby mamedev#131, mamedev#132) [Ignacio Prini]
- El Alambrista (type-in, MicroHobby mamedev#133) [Ignacio Prini]
- Ranamirez (type-in, MicroHobby mamedev#134) [Ignacio Prini]
- Fórmula Suicida (type-in, MicroHobby mamedev#135) [Ignacio Prini]
- Phantasmas (type-in, MicroHobby mamedev#136) [Ignacio Prini]
- Dardos (type-in, MicroHobby mamedev#137) [Ignacio Prini]
- S.E.M.I.S.I.S. (type-in, MicroHobby mamedev#138, mamedev#139) [Ignacio Prini]
- Alley's Gun (type-in, MicroHobby mamedev#140) [Ignacio Prini]
- Kleingeld (type-in, MicroHobby mamedev#141, mamedev#142) [Ignacio Prini]
- Caribe’s Day (type-in, MicroHobby mamedev#143) [Ignacio Prini]
- Rally (type-in, MicroHobby mamedev#144) [Ignacio Prini]
- La Profecía (type-in, MicroHobby mamedev#145) [Ignacio Prini]
- Mine Alert (type-in, MicroHobby mamedev#146, mamedev#147) [Ignacio Prini]
- Freddy (type-in, MicroHobby mamedev#148) [Ignacio Prini]
- Russian's Attack (type-in, MicroHobby mamedev#149, mamedev#150) [Ignacio Prini]
- En Ruta (type-in, MicroHobby mamedev#151) [Ignacio Prini]
- Zinco (type-in, MicroHobby mamedev#152) [Ignacio Prini]
- Ruffo’s Dream (type-in, MicroHobby mamedev#153) [Ignacio Prini]
- Crozet (type-in, MicroHobby mamedev#154) [Ignacio Prini]
- Rock Animation (type-in, MicroHobby mamedev#155) [Ignacio Prini]
- Bunker (type-in, MicroHobby mamedev#156) [Ignacio Prini]
- Telepuzzle (type-in, MicroHobby mamedev#157) [Ignacio Prini]
- Golfo Pérsico (type-in, MicroHobby mamedev#158) [Ignacio Prini]
- Mad Drivers (type-in, MicroHobby mamedev#159) [Ignacio Prini]
- Bowling Star (type-in, MicroHobby mamedev#160) [Ignacio Prini]
- Gas-Car (type-in, MicroHobby mamedev#161) [Ignacio Prini]
- Ghost Like (type-in, MicroHobby mamedev#162) [Ignacio Prini]
- Sky Invaders (type-in, MicroHobby mamedev#163) [Ignacio Prini]
- Perico Jones (type-in, MicroHobby mamedev#164) [Ignacio Prini]
- Brad Zotes (type-in, MicroHobby mamedev#165) [Ignacio Prini]
- Striker (type-in, MicroHobby mamedev#166) [Ignacio Prini]
- Indy (type-in, MicroHobby mamedev#167) [Ignacio Prini]
- Topin (type-in, MicroHobby mamedev#168) [Ignacio Prini]
- Starlike (type-in, MicroHobby mamedev#169) [Ignacio Prini]
- Tres en Raya (type-in, MicroHobby mamedev#170) [Ignacio Prini]
- Goteras (type-in, MicroHobby mamedev#171) [Ignacio Prini]
- Parvision (type-in, MicroHobby mamedev#172) [Ignacio Prini]
- Furax (type-in, MicroHobby mamedev#173) [Ignacio Prini]
- Duck Shooting (type-in, MicroHobby mamedev#174) [Ignacio Prini]
- Nomen Rosae (type-in, MicroHobby mamedev#175) [Ignacio Prini]
- PunkyMan (type-in, MicroHobby mamedev#176) [Ignacio Prini]
- El Retorno del Yedi (type-in, MicroHobby mamedev#177) [Ignacio Prini]
- Sky Warrior (type-in, MicroHobby mamedev#178) [Ignacio Prini]
- Reptkon (type-in, MicroHobby mamedev#179) [Ignacio Prini]
- Intro (type-in, MicroHobby mamedev#180) [Ignacio Prini]
- Keops (type-in, MicroHobby mamedev#181) [Ignacio Prini]
- Sir Gawain (type-in, MicroHobby mamedev#182) [Ignacio Prini]
- Snake (type-in, MicroHobby mamedev#183) [Ignacio Prini]
- Microbowl (type-in, MicroHobby mamedev#184) [Ignacio Prini]
- God Save the Punk (type-in, MicroHobby mamedev#185) [Ignacio Prini]
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.

4 participants