make check fails with:

Testing Complete
Number of tests: 3289
Number of todos: 198
Tests failed: 1
test/testvalueflow.cpp:1130(TestValueFlow::valueFlowBeforeConditionFunctionCall): Assertion failed. 
Expected: 
1
Actual: 
0

The relevant test is:

        code = "void f(char* x) {\n"
               "  strcpy(x,\"abc\");\n"
               "  if (x) {}\n"
               "}";
        ASSERT_EQUALS(true, testValueOfX(code, 2U, 0));

I don't really get it. This test seems to depend on the direction of the first argument, which i set to "out".

I don't really get it. This test seems to depend on the direction of the first argument, which i set to "out".

I did not check carefully but I assume valueFlowBeforeCondition would assume that x is changed by strcpy now. while it's only *x that is changed.

maybe direction should only be for the argument itself. and we can invent some direction2 or direction-data or whatever (later?) to handle when the function writes *x.

@danmar: we could name it direction-ptr?

Or even better: we introduce a type-option. So each argument can be associated with its corresponding argument data type.

Hmm ok. My assumption was that the content is meant (memory where the pointer points to). I believe not-uninit also works in this way, but I am not sure.
An uninitialized pointer is always considered a bug i guess. And in the same way it makes no sense to specify the direction for the pointer itself or are there exceptions?

IMHO direction-ptr is misleading if the content is meant. direction-data would be good.
Most important is that it is well defined and documented somehow.

IIRC in the Microsoft SAL the direction is also specified for either the variable or the memory a pointer points to. But i have to read over it again. IMHO it would make sense to let Cppchecks library configuration work similar if there is not a good reason to do otherwise.

I believe not-uninit also works in this way, but I am not sure.

yes it does. it is confusing. I would not be against that we define some new better configuration names.. The <not-null> and <not-uninit> should still work also and have the same meaning as today.. at least during a transition period.

IIRC in the Microsoft SAL the direction is also specified for either the variable or the memory a pointer points to. But i have to read over it again. IMHO it would make sense to let Cppchecks library configuration work similar if there is not a good reason to do otherwise.

Yes that makes sense

Here is a link to the Microsoft SAL:
https://docs.microsoft.com/en-us/visualstudio/code-quality/understanding-sal?view=vs-2017
"out" always means that the function writes to the memory that a given pointer points to.
I think that otherwise nothing really can be marked with "out". Except maybe variables that are given to a function by reference.

So, how do we define the currently implemented "in", "out" and "inout" exactly? I would add a corresponding comment to the library.cpp or so.

It is fine to match the msdn SAL annotation. but we need to determine if x itself is changed or not. How about adding a "refout" (reference-output).

If you have this:

int x = 10;
setValue(x);
a = x;  // <- x has unknown value

Then the "refout" should be configured instead of "out".

I do not see why refout or something is necessary. If the argument is configured as out and a simple variable is given like in your example Cppcheck could be sure that it must be handed over by reference. What am i missing here?
Otherwise it is no problem to implement and use refout if it is not redundant.

ok.. well I am satisfied with this in/out/inout definition. We can probably configure 99% of the functions with that definition.

one case that cannot be configured then would for instance be:

int *p;
setPointer(p);  // <- assign p
*p = 3;

Ah ok. A reference to a pointer is a case i have not thought of. I am now pro refptr 😀

hmm.. we also have the option to add argument type information. if the argument type is "int *&" for a "out" parameter then we can assume that it might be changed. If the argument type is "int *" then it is obvious it won't be changed.

I would recommend that in the near future we will not configure the direction at all for "refptr" cases. If no configuration is seen cppcheck should make the safe guess. Then we should be able to configure 99% of the functions properly now.

Some day in the future we can add some "refout" or argument type info also..

Ok.
Is it ok if i add the possibility to specify the type of an argument like it is already done for the returnValue? Or should we wait until it really is used/implemented in Cppcheck?

And how can we fix this PR?
I will have a look at valueFlowBeforeCondition but i am not sure if i will understand it and be able to fix it.

And how can we fix this PR?

Can you try to change your fix in isVariableChangedByFunctionCall?

I am guessing that if you ignore the direction when tok->valueType()->pointer is non-zero... then it will work like before here for pointers. This function is, as far as I remember, intended to check if the variable/pointer itself is changed... this should probably be clarified somehow.

I am guessing that if you ignore the direction when tok->valueType()->pointer is non-zero... then it will work like before here for pointers. This function is, as far as I remember, intended to check if the variable/pointer itself is changed... this should probably be clarified somehow.

I have changed the code to this:

    if (!tok->function()) {
        // Check if direction (in, out, inout) is specified in the library configuration and use that
        if (!addressOf && settings) {
            // With in, out, inout the direction of the content is specified, not for a pointer itself
            const ValueType * const valueType = tok->valueType();
            if (valueType && !valueType->pointer) {
                const Library::ArgumentChecks::Direction argDirection = settings->library.getArgDirection(tok, 1 + argnr);
                if (argDirection == Library::ArgumentChecks::Direction::DIR_IN)
                    return false;
                else if (argDirection == Library::ArgumentChecks::Direction::DIR_OUT ||
                         argDirection == Library::ArgumentChecks::Direction::DIR_INOUT)
                    return true;
            }
        }

But this can not be correct i think.
tok->valueType() always returns nullptr, so the whole direction check is disabled by this change.
Do i have to call valueType() on another token?

Do i have to call valueType() on another token?

Aha .. sorry my mistake. The tok is changed, I did not count with that. I believe you should use the original tok input that is put into the function. That should point at the variable. It seems you can use tok1. The ValueType should not usually be nullptr. Maybe there is some exception if we can see that there is a variable, but it has unknown type.

Thanks. With tok1 it works. The valueType information looks correct now.
But ignoring all pointers now practically disables the argument direction information in the library.
In testrunner this information is now not used at all.

I have added a test to verify that the direction is correctly loaded from the library configuration.

But ignoring all pointers now practically disables the argument direction information in the library.
In testrunner this information is now not used at all.

ok I guess most tests call some function dostuff or something and then we only have assumptions.

Here is a code example:

void f()
{
   char arr[10];
   char c='A';
   a = isdigit(c);
   arr[c] = 'x';
}

Cppcheck writes an inconclusive warning for this about array index out of bounds. But if the std.cfg says that isdigit parameter is in it should not be inconclusive.

I hope this can be fixed by your code.

Cppcheck writes an inconclusive warning for this about array index out of bounds. But if the std.cfg says that isdigit parameter is in it should not be inconclusive.

Yes, that really works. The inconclusive goes away :)
That would be a good candidate for a test i think. But i am not sure where to add it (testbufferoverrun.cpp? testlibrary.cpp?) since it tests several things.

Many of the functions in the tests are named foo(), dostuff() and so on. But for example strlen() is also used but the direction for the const "in" argument is ignored because it is a pointer.

Yes it's convoluted, not sure if we can "reduce it". otherwise I suggest that it is added to test/cfg/std.c ..

I also think that testastutils should have simple tests to ensure the function returns false when the direction is in (no matter if it's a pointer or not?).

Yes it's convoluted, not sure if we can "reduce it". otherwise I suggest that it is added to test/cfg/std.c ..

test/cfg/std.c sounds good, i will add it there.

I also think that testastutils should have simple tests to ensure the function returns false when the direction is in (no matter if it's a pointer or not?).

I also thought about returning false when it is an "in" argument even when it is a pointer.
Currently the direction is completely ignored for pointers. Would it be ok to change it to ignore directions only for "out" arguments?

Would it be ok to change it to ignore directions only for "out" arguments?

if you mean "inout" also.. then yes!

as far as I know we use definitions here that are compatible with Microsoft SAL. So we could extract the SAL definitions from the microsoft headers and put the information in our cfg files. If we write a tool that does this then maybe users could use that on their own code even and get automatically generated cfg files.

Would it be ok to change it to ignore directions only for "out" arguments?

if you mean "inout" also.. then yes!

yes, of course. I will change it.

as far as I know we use definitions here that are compatible with Microsoft SAL. So we could extract the SAL definitions from the microsoft headers and put the information in our cfg files. If we write a tool that does this then maybe users could use that on their own code even and get automatically generated cfg files.

IMHO that is a hard task. It would be great if we have such a tool, but it really is not easy to implement i guess. The headers are complex and so is the Microsoft SAL. It is not impossible, but a huge task i think.

@danmar I think this PR is ready to merge, do you agree?