ATHandler.cpp - Bad logical comparison

[TacoGrandeTX]

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

[cmonr]

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.

[ciarmcom]

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

[cmonr]

Closed #9018, since this was apparently intended.

Another way of fixing this behavior would be this comment: #8350 (comment)

@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.

[marcemmers]

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;
}

[mirelachirica]

Indeed, reason was what @marcemmers has supposed, to not duplicate the code needed in both matching cases c == tag[match_pos] and c == 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.

[0xc0170]

@TacoGrandeTX Thanks for spotting this !

@mirelachirica please send a fix