Fix enumeration of avr8 register index values

[felipesanches]

No description provided.

[cuavas]

The header changes are OK but the changes to the source are dubious at best. Also please stop mixing tab and space indentation.

[felipesanches]

The enumeration fix results in the replicator1 driver printing the unimplemented warning thousands of times. The patch provided here makes sure that the unimplemented warning shows up, but only once.

Also, it disables other timer #5 related log messages, since these are there only for debugging purposes and should not be enabled by default.

[felipesanches]

what does MAME use as a default for indentation: tabs or spaces? If it is spaces, then how many spaces per indentation level?

[balr0g]

I'm not sure if there are particular guidelines but generally MAME code is periodically run through srcclean (one of the tools built with make TOOLS=1), which cleans up spaces/tabs and such.
It probably would be worthwhile to write up a document describing what srcclean does.

[felipesanches]

it would be really good to specify a default tab/spaces style for everybody to use always. Letting scripts cleanup the whole codebase will probably messup with git blame, for instance. I'd rather preffer to setup my code editor with one good setting and make sure none of my commits mess up with indentation any more.

[cuavas]

MAME code generally uses tabs for initial indents only and spacing for other alignment. View the code with four spaces per tab if you want to see it aligned properly. You can work this out by opening pretty much any driver source file.
Disabling the logging for timer 5 by default (changing the #define to 0 rather than 1) may be acceptable, but using a static variable to suppress logging like that is most definitely unacceptable. The solution is to implement the missing modes. Otherwise leave it as is.

[felipesanches]

I'm curious to understand the rationale behind the idea of it being considered "most definitely unacceptable" to you. What are the bad consequences of it? Isn't it actually doing what it is designed to do? What are the drawbacks that make it so bad in your eyes?

I'm just asking because it sounds a bit harsh when someone calls something unnaceptable, so I'd expect to learn more about your reasoning. After all... it is just a simple "unimplemented" log message. Can't be that harmfull at all... :-)

[balr0g]

@cuavas: IMHO the very basic, most important things about source code formatting (such as that) should be in the top-level readme, in a section labeled "Contributing". I'm not sure what they are or I'd add them.

@felipesanches: other than it being rather ugly (at least to me), I'm not sure. That missing mode probably should be implemented, but I'll let @cuavas answer your question.

[etabeta78]

On Sun, May 17, 2015 at 1:19 AM, Felipe Corrêa da Silva Sanches <
notifications@github.com> wrote:

I'm curious to understand the rationale behind the idea of it being
considered "most definitely unacceptable" to you. What are the bad
consequences of it? Isn't it actually doing what it is designed to do? What
are the drawbacks that make it so bad in your eyes?

it's not bad in his eyes, it's against the direction MAME and MESS have
been moving in the past 4 years.
if you go back looking to the core changes made when adding the driver
states first, and the driver classes later (when passing to C++), and the
device-ification of various components, you will see that we have kept
removing static variables in favor of class variables.
the reason is that we aim to reach the point of being able to run multiple
MAME sessions on a single machine (e.g. to connect two arcade cabinet or
two consoles) and static variables risk to make this kind of scenario
unreliable.

[cuavas]

I don't see what you're problem with the word "unacceptable" is. The code will not be accepted by MAMEdev in its current form -- it is by definition "unacceptable". This is basic English. If you expect everyone to go out of my way not to say anything negative, you’re coming to the wrong place. It's my job to ensure contributions don't lower standards; ensuring fragile egos aren't bruised is not my job.

First of all, if you're thinking of using a function static variable, you're probably doing it wrong. It introduces inaccessible state within a function. It won't handle multiple instances, and it won't even handle multiple runs within the same process.

But putting that aside, you've completely changed what the statement does. The statement is there to log each time the software tries to use an unimplemented timer mode, and what the mode is each time. You've changed it to only log the first unimplemented timer mode used. That's far less useful as with your change one can no longer take the output, pass it through a text filter and get a list of all unimplemented timer modes used by a system. Your change doesn't bring the code any closer to a working state, and just attempts to put brokenness out of sight and out of mind. It doesn't help in any way. It might be OK to change it from a printf to osd_printf_error, but simply suppressing it like that is not a useful change, even ignoring the unacceptable implementation.

If you don't want to cooperate, I'll just close this pull request. It's less work for me than writing long-winded explanations, anyway.

[felipesanches]

Thanks for providing a clearer reasoning. I think it makes sense and I can (and will) adapt the code in this pull request accordingly.

More specifically, the consideration presented by @etabeta78 was something I wasn't aware of yet.

Finally, I'd like to make it clear that I want indeed to collaborate. My only concern was that it was not clear exactly what the criticism was. Now I understand it. Thanks.

[felipesanches]

I've just updated the commit, removing the static variable.
I also took the opportunity to fix the indentation levels in that area of the code.

Please review and merge if considered appropriate.

cheers,
Felipe

[cuavas]

The changes to indentation are not good:

• You are using mixed spaces and tabs for indentation - initial indent should just use tabs.
• Case labels by themselves do not introduce a scope level and therefore shouldn't cause indentation.

Switch statements should look like this:

switch (expr)
{
case val1:
case val2:
    unscoped;
    break;
case val3:
    {
        scoped;
    }
    break;
}

The switch statement itself and the compound statement inside it introduce scope. Case labels by themselves do not.

[balr0g]

@cuavas has updated the README to mention indentation standards (which should make it easier for contributors). Generally it's best to not muck too much with indentation; I usually do a git diff after making changes to ensure this is the case.

As for this? I'd just do the register enumeration and that's all.

[felipesanches]

I have just updated the commit to include exclusivelly the registers indexing fixes and the minor handling of debug messaging to avoid flooding the terminal. Log messages will only show up if a developer turn it on by setting the appropriate logging #define.

All fixes to broken indentation were now intentionally left aside, so that we can move on to merge this tiny avr8 bugfix.

[felipesanches]

Nevermind... I just noticed that the registers fixes were already merged into master. The log flood issue still remains, though. So I opened a new pull request specifically for that:

#186

[cuavas]

You'll be credited with the register fixes.

[felipesanches]

Thanks :-)
Sorry if I perhaps ended up sounding a bit harsh/arrogant in this thread. It was not my intention and I appologize.

Happy hacking!