Beckhoff / ADS

Beckhoff protocol to communicate with TwinCAT devices.
MIT License
491 stars 193 forks source link

GOTO in NotificationDispatcher.cpp #196

Closed ViaudJV closed 1 year ago

ViaudJV commented 1 year ago

in the file NotificationDispatcher.cpp. there is a Goto instruction in void NotificationDispatcher::Run() method.

void NotificationDispatcher::Run()
{
    for ( ; ; ) {
        sem.acquire();
        if (stopExecution) {
            return;
        }
        auto fullLength = ring.ReadFromLittleEndian<uint32_t>();
        const auto length = ring.ReadFromLittleEndian<uint32_t>();
        (void)length;
        const auto numStamps = ring.ReadFromLittleEndian<uint32_t>();
        fullLength -= sizeof(length) + sizeof(numStamps);
        for (uint32_t stamp = 0; stamp < numStamps; ++stamp) {
            const auto timestamp = ring.ReadFromLittleEndian<uint64_t>();
            const auto numSamples = ring.ReadFromLittleEndian<uint32_t>();
            fullLength -= sizeof(timestamp) + sizeof(numSamples);
            for (uint32_t sample = 0; sample < numSamples; ++sample) {
                const auto hNotify = ring.ReadFromLittleEndian<uint32_t>();
                const auto size = ring.ReadFromLittleEndian<uint32_t>();
                fullLength -= sizeof(hNotify) + sizeof(size);
                const auto notification = Find(hNotify);
                if (notification) {
                    if (size != notification->Size()) {
                        LOG_WARN("Notification sample size: " << size << " doesn't match: " << notification->Size());
                        goto cleanup;
                    }
                    notification->Notify(timestamp, ring);
                } else {
                    ring.Read(size);
                }
                fullLength -= size;
            }
        }
cleanup:
        ring.Read(fullLength);
    }
}

if think it's to remove goto and use break instead.

void NotificationDispatcher::Run()
{
    for ( ; ; ) {
        sem.acquire();
        if (stopExecution) {
            return;
        }
        auto fullLength = ring.ReadFromLittleEndian<uint32_t>();
        const auto length = ring.ReadFromLittleEndian<uint32_t>();
        (void)length;
        const auto numStamps = ring.ReadFromLittleEndian<uint32_t>();
        fullLength -= sizeof(length) + sizeof(numStamps);
        for (uint32_t stamp = 0; stamp < numStamps; ++stamp) {
            bool quit_cleanup = false;
            const auto timestamp = ring.ReadFromLittleEndian<uint64_t>();
            const auto numSamples = ring.ReadFromLittleEndian<uint32_t>();
            fullLength -= sizeof(timestamp) + sizeof(numSamples);
            for (uint32_t sample = 0; sample < numSamples; ++sample) {
                const auto hNotify = ring.ReadFromLittleEndian<uint32_t>();
                const auto size = ring.ReadFromLittleEndian<uint32_t>();
                fullLength -= sizeof(hNotify) + sizeof(size);
                const auto notification = Find(hNotify);
                if (notification) {
                    if (size != notification->Size()) {
                        LOG_WARN("Notification sample size: " << size << " doesn't match: " << notification->Size());
                        quit_cleanup = true;
                        break;
                    }
                    notification->Notify(timestamp, ring);
                } else {
                    ring.Read(size);
                }
                fullLength -= size;
            }
            if(quit_cleanup){
                 break;
            }

        }
        ring.Read(fullLength);
    }
}
pbruenn commented 1 year ago

Unless, you convince me with better code, I keep everything as is. The goto is there on purpose.