Mbed-os: ATHandler.cpp - Bad logical comparison

Created on 7 Dec 2018 · 7Comments · Source: ARMmbed/mbed-os

### Description

On a recent build off master I noticed this warning:

[Warning] app_util_platform.h@149,0:  #47-D: incompatible redefinition of macro "PACKED"  (declared at line 411 of "./mbed-os/platform/mbed_toolchain.h")
[Warning] ATHandler.cpp@962,0:  #187-D: use of "=" where "==" may have been intended

It sure looks like we intended this instead:

 } else if (c == tag[match_pos] || ((match_pos == 1) && (c == tag[--match_pos]))) {

Not familiar with cellular, so will let someone else PR this if required.

Issue request type

[ ] Question
[ ] Enhancement
[x] Bug

CLOSED mirrored bug

Source

TacoGrandeTX

Most helpful comment

I am not from mbed-os-wan so i can't say exactly but is suppose it was just the simplest way to not have the same code twice. However, rewriting the function seems to be a bit easier to read to me. Also moved the strlen() to the beginning so it only has to be calculated once. This is not tested but should have the same functionality.

bool ATHandler::consume_to_tag(const char *tag, bool consume_tag)
{
    size_t match_pos = 0;
    size_t tag_length = strlen(tag);

    while (true) {
        int c = get_char();
        if (c == -1) {
            tr_debug("consume_to_tag not found");
            return false;            
        }
        if (c == tag[match_pos]) {
            match_pos++;
        }
        else if (match_pos != 0) {
            match_pos = 0;
            if (c == tag[match_pos]) {
                match_pos++;
            }
        }
        if (match_pos == tag_length) {
            break;
        }        
    }

    if (!consume_tag) {
        _recv_pos -= tag_length;
    }
    return true;
}

marcemmers on 11 Dec 2018

👍3

All 7 comments

Nice catch! I've created a quick PR for the fix. This _definitely_ feels wrong, so let's see what the wan team has to say.

cmonr on 7 Dec 2018

Internal Jira reference: https://jira.arm.com/browse/MBOCUSTRIA-287

ciarmcom on 7 Dec 2018

Closed #9018, since this was apparently intended.

Another way of fixing this behavior would be this comment: https://github.com/ARMmbed/mbed-os/pull/8350#discussion_r231109684

@ARMmbed/mbed-os-wan I have to ask. Why is an assignment _inside_ of the conditional statement needed? This feels very wrong from a code quality standpoint.

cmonr on 10 Dec 2018

bool ATHandler::consume_to_tag(const char *tag, bool consume_tag)
{
    size_t match_pos = 0;
    size_t tag_length = strlen(tag);

    while (true) {
        int c = get_char();
        if (c == -1) {
            tr_debug("consume_to_tag not found");
            return false;            
        }
        if (c == tag[match_pos]) {
            match_pos++;
        }
        else if (match_pos != 0) {
            match_pos = 0;
            if (c == tag[match_pos]) {
                match_pos++;
            }
        }
        if (match_pos == tag_length) {
            break;
        }        
    }

    if (!consume_tag) {
        _recv_pos -= tag_length;
    }
    return true;
}

marcemmers on 11 Dec 2018

👍3

Indeed, reason was what @marcemmers has supposed, to not duplicate the code needed in both matching cases c == tag[match_pos] andc == tag[0].
Above proposal looks good and can be taken in use if
else if (c == tag[match_pos] || c == tag[match_pos = 0]) is not acceptable.