assert and fixed some flawed known value usage

[firewave]

No description provided.

[firewave]

This fails in at least TestLeakAutoVar::assign26. It has two values - the first is INT and Possible, and the other is BUFFER_SIZE and Known.

[firewave]

@pfultz2 Should this be actually checking for a known INT value?

[firewave]

@pfultz2 ping

[pfultz2]

Well its checking the condition based on a known int value or known tok value. I dont know if the known tok value is needed anymore.

[firewave]

Oh - I didn't look at the subsequent code. Sorry about that.

Based on that I think it should be simply calling getKnownValue(). Will give it a spin tomorrow.

[firewave]

Tests still pass with the change.

[danmar]

this code is technically redundant.

[danmar]

this redundant code is still here.

    if (mImpl->mValues->empty())
        return nullptr;

was it kept by intention?

[firewave]

How is it redundant? It might not be set or it might be empty. That is not the same. Or do you mean that the function as a whole is mostly redundant to the other one.

But it is still WIP. I forgot to mark it as draft again.

[danmar]

This:

    if (mImpl->mValues->empty())
        return nullptr;
    auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
        return value.isKnown();
    });
    return it == mImpl->mValues->end() ? nullptr : &*it;
}

Is logically the same if the first 2 lines are removed:

    auto it = std::find_if(mImpl->mValues->begin(), mImpl->mValues->end(), [&](const ValueFlow::Value& value) {
        return value.isKnown();
    });
    return it == mImpl->mValues->end() ? nullptr : &*it;

if mImpl->mValues is empty then it will be mImpl->mValues->end() and that means nullptr is returned.

So I suggest to remove the extra code.

[firewave]

Now I get it.

That would not be the same since the lamdba might still be created even though it is never used. I think I saw a similar optimization somewhere recently (but it is possible that was copying data in the capture). Need to have a look at the generated code and/or profile it.

[danmar]

@pfultz2 @firewave I don't feel I fully remember the details. do we want to enforce that the known value is always last? if we see such code maybe we should start making it more flexible instead?

[firewave]

The code contradicts itself. It checks if there is a known value from the front but then always uses the last value which might not be known. The assert is to detect such a case.

Based on the check it should also be calling getKnownValue() but I do not want to change that blindly without a case hitting it.

[danmar]

If I comment out these lines :

                //const ValueFlow::Value& value = arg1->values().back();
                //if (value.isTokValue() && value.tokvalue->tokType() == Token::eString)
                //    sizeValue = Token::getStrLength(value.tokvalue) + 1; // Add one for the null terminator

Then this test fails: TestValueFlow::valueFlowDynamicBufferSize
So this code is hit by a test case.

[firewave]

This is about some input hitting the wrong assumption and not the code in general.

[pfultz2]

It is not enforced to be the last element here, so this is definitely a bug. It should call getKnownValue(Value::ValueType::TOK).

[firewave]

Thanks for the clarification.

[firewave]

Done. All tests still pass.

[pfultz2]

This should use getKnownValue(Value::ValueType::TOK) and getKnownValue(Value::ValueType::INT) as getKnownValue can return a different type so it wont use the tokvalue if when its available.

In this case, since we prefer intvalue over tokvalue, we could make a getKnownValue that takes an initializer list and returns the first type found in the list getKnownValue({Value::ValueType::INT, Value::ValueType::TOK}):

const ValueFlow::Value* Token::getKnownValue(std::initializer_list<Value::ValueType> types) const
{
    for(auto t:types) {
        const ValueFlow::Value* value = getKnownValue(t);
        if(value)
            return value;
    }
    return nullptr;
}

[firewave]

Thanks for the clarification.

This might cause multiple lookups which would be bad in a hot path - see #6701 for such a case. But correctness is obviously first priority. So we could adjust that later on in the other PR.