MMIO: Cleanup Mapping class by using templates instead of macros.

[neobrain]

The MMIO mapping interface was using lots of macros before to use the same code for different MMIO register sizes. This branch replaces the macro usage with templates.

Macros aren't a particular modern way of using C++ to begin with, but in this case I think using templates instead of macros indeed makes the code a lot more readable. Additionally, I was able to clean up the interface a lot because the C++ overloading restrictions don't affect templated code.

For what it's worth, I'm not sure if

template<typename Unit>
struct HandlerArray
{
    using Read = std::array<ReadHandler<Unit>, NUM_MMIOS / sizeof(Unit)>;
    using Write = std::array<WriteHandler<Unit>, NUM_MMIOS / sizeof(Unit)>;
};

HandlerArray<u8>::Read m_read_handlers8;
HandlerArray<u16>::Read m_read_handlers16;
HandlerArray<u32>::Read m_read_handlers32;

HandlerArray<u8>::Write m_write_handlers8;
HandlerArray<u16>::Write m_write_handlers16;
HandlerArray<u32>::Write m_write_handlers32;

is any cleaner than

std::array<ReadHandler<u8>, NUM_MMIOS / sizeof(Unit)> m_read_handlers8;
std::array<ReadHandler<u16>, NUM_MMIOS / sizeof(Unit)> m_read_handlers16;
std::array<ReadHandler<u32>, NUM_MMIOS / sizeof(Unit)> m_read_handlers32;

std::array<WriteHandler<u8>, NUM_MMIOS / sizeof(Unit)> m_write_handlers8;
std::array<WriteHandler<u16>, NUM_MMIOS / sizeof(Unit)> m_write_handlers16;
std::array<WriteHandler<u32>, NUM_MMIOS / sizeof(Unit)> m_write_handlers32;

Opinions on that? (having written this in comparison, I got to say myself that the second version is probably better)

For reference, the version of this code living in current master is

#define HANDLERS(Size) \
std::array<ReadHandler<u##Size>, NUM_MMIOS / sizeof (u##Size)> m_Read##Size##Handlers; \
std::array<WriteHandler<u##Size>, NUM_MMIOS / sizeof (u##Size)> m_Write##Size##Handlers;
HANDLERS(8) HANDLERS(16) HANDLERS(32)
#undef HANDLERS

[neobrain]

Pushed a new version, which fixes the MMIO UnitTest. This also nicely illustrates one of the interface changes.

[neobrain]

I'm not yet sure what's happening on the OSX build bot, but I'll look into it.

[BhaaLseN]

I kinda like the first version better...altho it could be more readable (as in words, not code):
Write<u16>::Handlers m_write_handlers16, WriteHandler<u16>::Array m_write_handlers16 or something like that...
Its understandable enough as to what it should do/be, and if you want the gory details you could look at the struct.

[delroth]

The second version is in no way what is committed in the repository.
Comparing your rewrite to a non existing, repetitive version to make it
look better is not very honest.
On Jul 12, 2014 2:55 PM, "BhaaL" notifications@github.com wrote:

I kinda like the first version better...altho it could be more readable
(as in words, not code):
Write::Handlers m_write_handlers16, WriteHandler::Array
m_write_handlers16 or something like that...
Its understandable enough as to what it should do/be, and if you want the
gory details you could look at the struct.

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

[neobrain]

I think you misunderstood the point of that comparison. I wasn't pointing out how the new code is superior to the old one, but instead I was asking for opinions on how to replace the old macro code with non-preprocessor code. In any case, both versions should be equivalent to the old code unless I am missing something.

EDIT: Added the old code to the comparison for reference.

[delroth]

What's the status of this? OSX is still not building, and no status on compiler code being emitted. Please do not abuse the PR list as your personal TODO list.

[neobrain]

At the time I submitted this PR I thought everything would be fine, so it's not an abuse of the PR list as a TODO list.

I haven't had some time to work on this any further yet, but I'll get to it eventually.

[neobrain]

(I don't know why the buildbot is showing success; the OSX buildbot still fails)

I think I might be hitting this compiler bug: http://llvm.org/bugs/show_bug.cgi?id=18345
Not quite sure what to do about this, yet.

[neobrain]

Okay, should be final now. It's not very pretty, but it compiles on clang and maybe works even better than the previous version. If there are no reasons not to merge this, I'd stash the two commits together and then merge this.

[neobrain]

If this against my expectations causes any performance regressions, we can just revert it. However, currently it looks like the new version is completely equivalent to the old one with regards to the generated assembly code.

[shuffle2]

why is it so mysterious? just look at the compiled code.

On Sun, Jul 27, 2014 at 10:28 AM, Tony Wasserka notifications@github.com
wrote:

If this against my expectations causes any performance regressions, we can
still revert it. However, currently it looks like the new version is
completely equivalent to the old one.

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

[neobrain]

What are you talking about? I did look at the compiled code, as explained in the commit comments (.. the link to which I unfortunately don't have anymore because it got lost during rebase), and couldn't see any relevant differences in the generated assembly code.

[shuffle2]

OK....I missed the response on the pre-rebase version.