MemoryLayout constructor changes

[pvmm]

MemoryLayout constructor has code such as:

        primarySlot[p] = '0';
        secondarySlot[p] = 'X';

which tells me that they store ASCII code. But then, everywhere else MemoryLayout::{primary, secondary}Slot are treated as byte-sized integers. This is confusing and error prone. I am surprised it didn't burst into flames when I found it, since there are loads of code that do this:

        if (bp.ps == -1 || bp.ps == memLayout->primarySlot[page])
...
        if (bp.ss == -1 || bp.ss == memLayout->secondarySlot[page])

but then I found later on that SlotViewer::slotsUpdated() sets MemoryLayout::{primary, secondary}Slot as byte-sized integers as well after the first debug step:

        memLayout->primarySlot  [p] = lines[p * 2][0].toLatin1()-'0';
        memLayout->secondarySlot[p] = lines[p * 2][1].toLatin1()-'0';

So this PR changes MemoryLayout constructor to use the same data representation. And it also replaces char by qint8 to emphasize this distinction in other places.

[m9710797]

Hi, Definitely an improvement. But could you check also these two items:

• SlotViewer.cpp:205 This still does an ASCII-based calculation for secondarySlot[], is that (still) correct?
• DebuggerForm.cpp:1498 (Detail) Can this be simplified to unconditionally return secondarySlot[p]? In other words: is it now guaranteed that "isSubSlotted[primarySlot[p]] == false" implies "secondarySlot[p] == -1". (Would be good to document these invariants).

If you can check these two items, I'll merge this PR. Thanks.

-- Optional: possible future enhancement: I still see other places where "ps, ss, segment" are passed as 3 integers (e.g. DebuggerForm.cpp:1493). It might make sense to define a small struct like:

struct SlotAndSegment {
    uint8_t ps; // always 0..3
    std::optional<uint8_t> ss; // either 0..3 or nullopt (when this primary slot has no subslots)
    std::optional<uint8_t> segment; // either 0..255 or nullopt (when this slot contains no memory mapper)
};

This documents the invariants, and e.g. it can be returned by value instead of via 3 out-parameters.

Detail: I personally prefer std::optional over a special value (like -1). Because it makes it harder to forget to check for this special case. Disadvantage is that it's slightly larger (sizeof(uint8_t) == 1, sizeof(std::optional)==2) and because of this maybe a tiny bit slower. But I think that doesn't matter in this context.

[pvmm]

SlotViewer.cpp:205 This still does an ASCII-based calculation for secondarySlot[], is that (still) correct?

Fixed it. It also receives 'X' when subslot is not set, so subtracting '0' doesn't produce the expected result. I also fixed this:

            text += QString("\nBinary: %1 %2 %3 %4")
                .arg((wd & 0xF000) >> 12, 4, 2, QChar('0'))
                .arg((wd & 0x0F00) >>  8, 4, 2, QChar('0'))
                .arg((wd & 0x00F0) >>  4, 4, 2, QChar('0'))
                .arg((wd & 0x000F) >>  0, 4, 2, QChar('0'));

which was causing warnings every time the user hovers a HexViewer.

QString::arg: Argument missing: "\nBinary: 0000 0000 0000 0000" , 0
QString::arg: Argument missing: "\nBinary: 0000 0000 0000 0000" , 0
QString::arg: Argument missing: "\nBinary: 0000 0000 0000 0000" , 0
QString::arg: Argument missing: "\nBinary: 1110 1110 1110 1110" , 6
QString::arg: Argument missing: "\nBinary: 1110 1110 1110 1110" , 0
QString::arg: Argument missing: "\nBinary: 1110 1110 1110 1110" , 3

[m9710797]

Hi,

I was about to merge this PR, but then you force-pushed another version, and another some minutes later. Let me know when you think this PR is ready. (I did a very quick scan over the code, and to me it looks good). Thanks again for your work!

Wouter

[pvmm]

Sorry about that. Not going to touch it again.

[m9710797]

Merged. Thanks.