bloguetronica / cp2130-qt

A C++ class that uses libusb to interface with CP2130 devices. It can be used to configure the OTP ROM of such devices, as well as to control them. The class was made for Qt. If you wish to use a derived non-Qt version, please refer to https://github.com/bloguetronica/cp2130.
0 stars 0 forks source link

PROMConfig "equal to" operator efficiency issue #5

Closed samuelfmlourenco closed 3 years ago

samuelfmlourenco commented 3 years ago

The PROMConfig "equal to" operator should return immediately when a difference is found. Only the inner for cycle is being interrupted. The offending code:

// "Equal to" operator for PROMConfig
bool CP2130::PROMConfig::operator ==(const CP2130::PROMConfig &other) const
{
    bool retval = true;
    for (size_t i = 0; i < PROM_BLOCKS; ++i) {
        for (size_t j = 0; j < PROM_BLOCK_SIZE; ++j) {
            if (blocks[i][j] != other.blocks[i][j]) {
                retval = false;
                break;
            }
        }
    }
    return retval;
}

The correct approach, while keeping one return point per function in order to respect the structured approach, should be this:

// "Equal to" operator for PROMConfig
bool CP2130::PROMConfig::operator ==(const CP2130::PROMConfig &other) const
{
    bool retval = true;
    for (size_t i = 0; i < PROM_BLOCKS; ++i) {
        for (size_t j = 0; j < PROM_BLOCK_SIZE; ++j) {
            if (blocks[i][j] != other.blocks[i][j]) {
                retval = false;
                break;
            }
        }
        if (!retval) {  // If "retval" is false, then a difference was already found
            break;
        }
    }
    return retval;
}

This will be fixed in the next release.

samuelfmlourenco commented 3 years ago

As implemented:

// "Equal to" operator for PROMConfig
bool CP2130::PROMConfig::operator ==(const CP2130::PROMConfig &other) const
{
    bool equal = true;
    for (size_t i = 0; i < PROM_BLOCKS; ++i) {
        for (size_t j = 0; j < PROM_BLOCK_SIZE; ++j) {
            if (blocks[i][j] != other.blocks[i][j]) {
                equal = false;
                break;
            }
        }
        if (!equal) {  // Added in version 2.0.1 in order to fix efficiency issue
            break;
        }
    }
    return equal;
}