fix #11824: Option --max-configs has no effect if -D is used

[clock999]

I have checked the comments on the ticket of the bug. And I simply summarized my logic of checking the configs as below.

The configs that will be totally checked are the sum of the user defined value and the default list. The user defined value and the default list are combined to a total list. The user defined value will be inserted into the total list ahead of the default list.
The user defined value could be empty or not only depends on the -D in the cmdline and the settings in the file imported by --project.
If the --max-config is specified, the total list will have a max length which is the --max-config.
If preprocessOnly is set, only one config in the total list is checked.

[firewave]

This needs a very thorough review as well as tests and documentation (in general and documenting this behavior change). As mentioned in the ticket we might need to fix other things first.

We might also need to print some messages/error if certain configurations are incompatible (I have not looked at it at all yet).

[danmar]

Basically I agree with the idea. If -DFOO --max-configs=2 is used then I would expect that 2 configurations that both define FOO is checked.

You can add an example that clarify this in the manual also:
https://github.com/danmar/cppcheck/blob/main/man/manual.md?plain=1#L386

[danmar]

I would also like to see some regression tests.

[danmar]

I don't remember why we have added Settings::force and Settings::checkAllConfigurations. Isn't it enough with just Settings::maxConfigs?

Imho testcmdlineparser.cpp should test that when --max-config=2 is used then Settings::maxConfigs will be 2 no matter if -D has been used or not.

[danmar]

This code will always execute Preprocessor::getConfigs even if max configs is 1.

[firewave]

I don't remember why we have added Settings::force and Settings::checkAllConfigurations. Isn't it enough with just Settings::maxConfigs?

I think that has been grown historically. It is also possible that part of this is cleaned up with #7293. The duplicated and differing logic in the CLI and GUI code make things a bit awkward.

[danmar]

The duplicated and differing logic in the CLI and GUI code make things a bit awkward.

yes.. if we can unify logic that would make me happy.. :-)

[firewave]

yes.. if we can unify logic that would make me happy.. :-)

Working on it...

[clock999]

yes.. if we can unify logic that would make me happy.. :-)

Working on it...

Hi firewave, If it is ok, I’d like to complish this PR. So if the document for how the configs should be handled is ready, could you please let me know that. Thanks a lot!

[danmar]

@clock999 sorry for late response. do you want to continue working on this? I would like that we have consistent proper behavior when --max-configs is used.

[clock999]

@clock999 sorry for late response. do you want to continue working on this? I would like that we have consistent proper behavior when --max-configs is used.

Hi Danmar,
Yes, I'd like to continue working on this and try to summarize a final clear logic. And maybe it's better we have a agreed basic rule for how we handle the configs. With the previous comments, seems some co-workers are working on a document or something about this. I have some basic thinking but I am not sure if you are agree or not.
Actually, --max-configs and -D should have nothing to do with each other. Currently, for example, if we have the options '-DAAA --force' to check below code.

int main() {
    #ifdef ABC1
    int abc1 = 1;
    #endif
    
    #ifdef ABC2
    int abc2 = 1;
    #endif
    
    #ifdef ABC3
    int abc3 = 1;
    #endif
    
    return 1;
}

We will have 4 checks. That is just our current behavior in fact.

AAA=1...
AAA=1;ABC1...
AAA=1;ABC2...
AAA=1;ABC3...

So if using the options '--max-configs=3 -DAAA', we just need to keep the behavior the same with using --force, and generate 3 checks which is just configured by the --max-configs.

AAA=1...
AAA=1;ABC1...
AAA=1;ABC2...

If the user want to only check the user-defined config, he can use --max-configs=1.

[clock999]

I think maybe we should remove Settings::checkAllConfigurations. Maybe GUI should only accept the raw user input to set the Settings, but not edit the Settings. As I understand, the values of the items of the Settings should all be read only, which are only decided by the user input. The Settings items on the UI shown to the user for selecting should be consistent with the CLI options, or can be directly converted to corresponding options.
I see there is below code in mainwindow.cpp:

        // Only check the given -D configuration
        if (!defines.isEmpty())
            settings.maxConfigs = 1;

        // If importing a project, only check the given configuration
        if (!mProjectFile->getImportProject().isEmpty())
            settings.checkAllConfigurations = false;

Since 'settings.maxConfigs = 1;' is used, why still setting 'settings.checkAllConfigurations = false;'? Maybe I missed some points. The '-D configuration' and 'the given configuration' are different? If we are using the GUI, can we still be able to have '-D configuration'?
I will check more code to be clear of the behaviors.
Anyway, as said at the beginning, I think maybe we should not set 'settings.maxConfigs = 1;', it is only decided by the user input. And we can use the value of the defines that is also input by the user.

[clock999]

I updated an original version, and will update it further.

[danmar]

As I understand, the values of the items of the Settings should all be read only, which are only decided by the user input.

this is more or less correct. For these preprocessor settings it is correct. Removing / renaming Settings members is not a big deal.. we can do that easily. And I am not sure why we have that checkAllConfigurations.

[danmar]

We will have 4 checks. That is just our current behavior in fact.

This is the expected behavior imho

So if using the options '--max-configs=3 -DAAA', we just need to keep the behavior the same with using --force, and generate 3 checks which is just configured by the --max-configs.

this sounds reasonable the --max-configs indicate that the user do want to check multiple configurations.

If the user want to only check the user-defined config, he can use --max-configs=1.

to be clear, I expect that this command:

cppcheck -DAAA test.c

should only check "AAA" configuration and nothing else.

If --max-configs=1 is used also cppcheck -DAAA --max-configs=1 test.c should this mean that a valid configuration is extracted? Let's say that the specific configuration "AAA" provided by the user leads to a preprocessorErrorDirective.. but Cppcheck determines that "AAA;FOO" is a valid configuration..

[clock999]

Hi Danmar,

        // If importing a project, only check the given configuration
        if (!mProjectFile->getImportProject().isEmpty())
            settings.checkAllConfigurations = false;

While setting the checkAllConfigurations, I see there is a comment "If importing a project, only check the given configuration".
So when a project file is imported, even though there is no defines in the project file, shall we ignore all the config checks?
Take the unit test(cppcheck/test/cli/helloworld) for example. We have a project file cppcheck/test/cli/helloworld/test.cppcheck:

<?xml version="1.0" encoding="UTF-8"?>
<project version="1">
  <paths>
    <dir name="."/>
  </paths>
  <addons>
    <addon>misra</addon>
  </addons>
</project>

With running "cppcheck --project=test/cli/helloworld/test.cppcheck", the expected result is that SOME_CONFIG is not checked, that is defined in the main.c.
Should we keep the current behavior? Maybe if there is no defines in the project file, we can check the default extracted configs just as there is no '-D' options.

[danmar]

So when a project file is imported, even though there is no defines in the project file, shall we ignore all the config checks?

Spontanously I feel that:

• --project=compile_commands.json => by default check only 1 configuration..
• --project=helloworld.cppcheck => check multiple configurations
• --project=some-other-project.cppcheck => it depends on if defines are configured or not, and if it imports for instance a compile database.

Maybe if there is no defines in the project file, we can check the default extracted configs just as there is no '-D' options.

Yes.. unless a compile_commands.json file is imported by the project.

[clock999]

Hi Danmar,
I write the details the unit test case below. So it is easy for the discussion.
Under the folder named 'helloworld', there are main.cpp, test.cppcheck and compile_commands.json.
main.cpp:

#include <stdio.h>

int main(void) {
    (void)printf("Hello world!\n");
    int x = 3 / 0; (void)x; // ERROR
    return 0;
}

#ifdef SOME_CONFIG
void foo();
#endif

test.cppcheck:

<?xml version="1.0" encoding="UTF-8"?>
<project version="1">
  <importproject>./compile_commands.json</importproject>
  <addons>
    <addon>misra</addon>
  </addons>
</project>

If running 'cppcheck --project=helloworld/test.cppcheck', do you think we should have below output:

Checking helloworld/main.cpp ...
helloworld/main.cpp:5:15: error: Division by zero. [zerodiv]
    int x = 3 / 0; (void)x; // ERROR
              ^

But why NOT:

Checking helloworld/main.cpp ...
helloworld/main.cpp:5:15: error: Division by zero. [zerodiv]
    int x = 3 / 0; (void)x; // ERROR
              ^
Checking helloworld/main.cpp: SOME_CONFIG...

Why whether import a compile_commands.json or not will impact the behavior of checking the configs. Seems it is not related with that.

[danmar]

Why whether import a compile_commands.json or not will impact the behavior of checking the configs. Seems it is not related with that.

In my humble opinion, when you import a compile_commands.json it is expected that we check the files and explicit configurations provided in the compile_commands.json. The Cppcheck analysis should match the compilation as much as we can in this case..

If --force or --max-configs is also used I am open to be more flexible..

[danmar]

In my humble opinion, when you import a compile_commands.json it is expected that we check the files and explicit configurations provided in the compile_commands.json. The Cppcheck analysis should match the compilation as much as we can in this case..

Also if there is a -D in a command in the json file then I believe we will check the exact provided configuration and the extra SOME_CONFIG is not checked. And the result could be that if some of the commands have a -D and other don't then we could get that extra configurations are checked in some files and not in other files..

[clock999]

Hi Danmar,
I think we have 3 ways to have user defined configs.

using -D in the command line
using the defines tag in the .cppcheck file
using -D in the "command": "/usr/bin/c++ -D" of the compile_commands.json

I tested the 3 ways. Each of the three will add the defined config to the Settings::userDefines. They have the same behavior.
For example:
compile_commands.json:

[
{
  "directory": "helloworld",
  "command": "/usr/bin/c++    -o CMakeFiles/main.dir/main.cpp.o -c helloworld/main.cpp -DJJJ",
  "file": "helloworld/main.cpp",
  "output": "CMakeFiles/main.dir/main.cpp.o"
}
]

test.cppcheck

<?xml version="1.0" encoding="UTF-8"?>
<project version="1">
  <importproject>./compile_commands.json</importproject>
  <addons>
    <addon>misra</addon>
  </addons>
  <defines>
    <define name="ABCDEF">100</define>
  </defines>
</project>

Running:
cppcheck --project=helloworld/test.cppcheck -DAAA
Then the Settings::userDefines will have the value: ABCDEF;AAA=1;JJJ=1
So I think we can make the 3 ways the same behavior. And even though there is a compile_commands.json imported, if there is no user defined configs, we can check the extracted configs with the default max numbers of 12, just as running a simple command 'cppcheck main.c' with a default behavior.

[clock999]

Hi Danmar,
Many thanks for your time with the comments for correcting my submit, that really help me a lot!
For this PR, I summarized the logic as below.

whether importing a project file or not will not impact the behavior of checking configs, we only care about if there is user defined configs or not in the imported project file, .cppcheck file or compile_commands.json .

the user defined configs, --max-configs, --force will decide the behavior of checking configs together.

with user_defined_configs, with not --max-configs, with not --force, only the user defined configs are checked with none of the extracted configs.

with user_defined_configs, with --max-configs, with --force, the user defined configs combined with the extracted configs are checked, the max number of all the checks is --max-configs.

with user_defined_configs, with not --max-configs, with --force, the user defined configs combined with the extracted configs are checked, the number of all the checks is the number of the extracted configs.

Do you have any further suggestion for this? I think maybe we can use this submit to do the fix for this bug.

[danmar]

And even though there is a compile_commands.json imported, if there is no user defined configs, we can check the extracted configs with the default max numbers of 12, just as running a simple command 'cppcheck main.c' with a default behavior.

no that was what I did not want in my comment #7424 (comment)

In my opinion we should check the explicit configuration provided in the compile_commands.json. By default there should only be 1 configuration checked even if some commands don't use any -D options.

[clock999]

Hi Danmar,
Suppose we have below scenary.
compile_commands.json:

[
{
  "directory": "helloworld",
  "command": "/usr/bin/c++    -o CMakeFiles/main.dir/main.cpp.o -c helloworld/main.cpp",
  "file": "helloworld/main.cpp",
  "output": "CMakeFiles/main.dir/main.cpp.o"
}
]

test.cppcheck

<?xml version="1.0" encoding="UTF-8"?>
<project version="1">
  <importproject>./compile_commands.json</importproject>
</project>

helloworld/main.cpp:

int main() {
    #ifdef ABC1
    int abc1 = 1;
    #endif
    
    #ifdef ABC2
    int abc2 = 1;
    #endif
    
    #ifdef ABC3
    int abc3 = 1;
    #endif
    
    return 1;
}

Run the command line:
cppcheck --project=test.cppcheck

That is to say we have no user defined config at all, and we have a compile_commands.json imported. Do you think we should only check the ABC1, or just run the check with an empty config define string. Actually, when getting the extracted the configs, we put an empty string on the first of the list.

std::set<std::string> Preprocessor::getConfigs(

I am not sure if I am using the correct word when saying the 'extracted the configs'. I just mean the configs returned by getConfigs.

[danmar]

Do you think we should only check the ABC1, or just run the check with an empty config define string. Actually, when getting the extracted the configs, we put an empty string on the first of the list.

I would like an "empty config string" in this case because that will match the compilation.

[clock999]

Hi Danmar,
I updated the commit.

the user_defined_configs are the configs in the imported files including compile_commands.json combined with the ones from the '-D' option

with compile_commands.json imported, with not --max-configs, with not --force, only one config is checked.

with compile_commands.json imported, with --max-configs or with --force, follow below rules with compile_commands.json or not.

with user_defined_configs, with not --max-configs, with not --force, only the user defined configs are checked with none of the extracted configs.

with user_defined_configs, with --max-configs, with --force, the user defined configs combined with the extracted configs are checked, the max number of all the checks is --max-configs.

with user_defined_configs, with not --max-configs, with --force, the user defined configs combined with the extracted configs are checked, the number of all the checks is the number of the extracted configs.

[danmar]

I thought there would be some update in the manual.. is it clear? Since we've had so much discussions I would think it should be updated.

[danmar]

ehm.. there is not much testing added for all these changes.

do you feel confident that all the cases you listed are tested properly? It might be interesting to add specific tests that are only meant to check that --max-configs, --force, -D, --project=.. results in correct behavior

[danmar]

this can be changed to a bool as far as I see.

[danmar]

This is a promising start. I would also like to see tests that check what happens when a --project=compile_commands.json is used.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[danmar]

well those CI failures needs some minor tweaks I assume?