I agree. I have seen linux programs that list obsolete command line arguments in the help text with a description like "Provided for backward compatibility. Does not do anything".
Not sure if that makes sense here and what Daniel thinks about it.
If it is simply ignored the behaviour could be really unexpected and a warning message could be missed. So maybe it makes sense in this case to really remove the argument?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
It was somehow removed by mistake as far as I can tell. There was no discussion to remove any parameters. The addon was refactored, and I guess I did not see that anything was removed. Please feel free to add it back. Anyway.. what was the purpose of -P?
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Awesome. That helps me test much more easily. A deprecation warning is usually the best way to communicate it's not needed anymore. It can be wrapped with --quiet so that it can be squelched if necessary.
The -P option was short for "--file-prefix" see the commit message for a detailed description of why I added it.
In short it allows you to have defaut paths in the suppressions file so you don't have to add them. They become relative to the root directory of the source tre.. This allows you to have the root of the source project in different locations yet use the same suppressions file.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The title of that PR is "MISRA: Fix rules suppression, add tests". So it fixed some problem and introduced others.
I think it will be too much work in the long run to have suppressions in the addons also. It's better that we only have it in Cppcheck. When suppressions was added there was no way to reuse the cppcheck builtin suppressions handling.. but now there is.
We should deprecate this.. but of course it will not just be removed or disabled.
I suggest we deprecate it now and say that these options will be removed in 1.95.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
What's the plan for this conversion? I'll need to plan a migration from transitioning from specifying this suppression in the misra.py call vs putting them in cppcheck.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I think it sounds good to deprecate it. We need to add some warning now when these options are used.. if you have a little time feel free to send us a PR.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Usually the deprecation warnings are after the replament is in place, functional, and the recomended alternative is documented.
Its not clear to me what the migraion path is. The manual says [error id]:[filename]:[line] but there's nothing that mentions what the [error id] would be for MISRA errors.
The manual goes on to say:
The error id is the id that you want to suppress. The easiest way to get it is
to use the –template=gcc command line flag. The id is shown in brackets.
But I don't see how that will provide userful information in the MISRA case.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
${PROJECT_TOOLS_DIRECTORY} is custom to each developer so it's not know until CMake builds the make files. I can't have a pre-set misra.json with a path because its different for each developers machine.
I suppose I could create a CMake step that builds a cusom misra.json but that seems cumbersome and I would not be able to keep it under source control.
To test the --addon=misra.json I hardcoded the path for my machine.
There is slightly different in output between the 2 methods.
Via: --addon=
[/home/rsmith/whoop/firmware/copley/src/console.c:48]: (style) The value returned by a function having non-void return type shall be used`
Via: --dump
[/home/rsmith/whoop/firmware/copley/src/console.c:48] (style) The value returned by a function having non-void return type shall be used (Required) [misra-c2012-17.7]
The --addon output is missing the Severity and the Rule.
Lasty there is a mismatch between the rule format you provided and what the rule paser actually accepts.
You specified:
--suppress=misra-c2012-18.4
But the parser expects rules in the following format:
misra_6.0
misra_7_0
misra.21.11
There is slightly different in output between the 2 methods.
Yes. The output will now be formatted the same way as normal cppcheck warnings. You can use --template to get the output you want.
But yes you are right that we need to append "(Required)" in the --cli output somehow. I'd say it would be best if it was a separate item in the json output. The misra addon is not allowed to change the texts in any way.
So well.. that will lead to some hacks in cli/gui.
If the format you provided is to be the offical format then the regex needs updating.
I think the new format should be official. But well do we need to update the parser? It just means you can't reuse your rules file as is. You will need to make simple edits.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
The path to the suppression liste is ok. Its the path to the --rule-texts= that I need to set.
But yes you are right that we need to append "(Required)" in the --cli output somehow. I'd say > it would be best if it was a separate item in the json output. The misra addon is not allowed to > change the texts in any way.
I'm not sure I follow yet. I'll play with the template output to better understand the limitation.
I think the new format should be official. But well do we need to update the parser? It just >means you can't reuse your rules file as is. You will need to make simple edits.
I'm talking about the format in my suppressons file not the rules-text file. An example is:
MISRA_13.4:sensors.c:110
If I change that to:
MISRA-c2012_13.4:sensors.c:110
Then the supression no longer works.
The supressions file is under source control so an update would need to be able to support both formats so it works on current and older revisions of the file.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
I just updated to HEAD to test if the syntax error was fixed and found that our setup was completely broken.
The -P option to misra.py was removed without any warning. Our build system used this argument.
Removing command line options without any warning or depecation period and no backwards compatiblilty is not very user friendly.
I agree. I have seen linux programs that list obsolete command line arguments in the help text with a description like "Provided for backward compatibility. Does not do anything".
Not sure if that makes sense here and what Daniel thinks about it.
If it is simply ignored the behaviour could be really unexpected and a warning message could be missed. So maybe it makes sense in this case to really remove the argument?
It was somehow removed by mistake as far as I can tell. There was no discussion to remove any parameters. The addon was refactored, and I guess I did not see that anything was removed. Please feel free to add it back. Anyway.. what was the purpose of -P?
There is already a PR for bringing it back: https://github.com/danmar/cppcheck/pull/2030
See also https://github.com/danmar/cppcheck/pull/1996#issuecomment-514188421
Awesome. That helps me test much more easily. A deprecation warning is usually the best way to communicate it's not needed anymore. It can be wrapped with --quiet so that it can be squelched if necessary.
The -P option was short for "--file-prefix" see the commit message for a detailed description of why I added it.
In short it allows you to have defaut paths in the suppressions file so you don't have to add them. They become relative to the root directory of the source tre.. This allows you to have the root of the source project in different locations yet use the same suppressions file.
The title of that PR is "MISRA: Fix rules suppression, add tests". So it fixed some problem and introduced others.
I think it will be too much work in the long run to have suppressions in the addons also. It's better that we only have it in Cppcheck. When suppressions was added there was no way to reuse the cppcheck builtin suppressions handling.. but now there is.
We should deprecate this.. but of course it will not just be removed or disabled.
I suggest we deprecate it now and say that these options will be removed in 1.95.
I Agree 100%. What I added was just a temporary feature because I needed it and it didn't exist.
I fixed it in https://github.com/danmar/cppcheck/pull/2030.
--file-prefix
option has been restored, with a small fix described here.I agree that the use of additional addon-specific suppressions is redundant. All the necessary suppression rules should be defined in a single place.
Let's warn the user about deprecation, when it use one of
--rule-suppression
,--suppress-rules
or--show-suppressed-rules
with addon cli?What's the plan for this conversion? I'll need to plan a migration from transitioning from specifying this suppression in the misra.py call vs putting them in cppcheck.
I think it sounds good to deprecate it. We need to add some warning now when these options are used.. if you have a little time feel free to send us a PR.
Usually the deprecation warnings are after the replament is in place, functional, and the recomended alternative is documented.
Its not clear to me what the migraion path is. The manual says [error id]:[filename]:[line] but there's nothing that mentions what the [error id] would be for MISRA errors.
The manual goes on to say:
The error id is the id that you want to suppress. The easiest way to get it is
to use the –template=gcc command line flag. The id is shown in brackets.
But I don't see how that will provide userful information in the MISRA case.
I think we now have the "replacement" working.
You can for instance suppress a warning like this:
It's close but not quite. I've found 3 items.
This is what my misra check via --dump looks like:
${PROJECT_TOOLS_DIRECTORY} is custom to each developer so it's not know until CMake builds the make files. I can't have a pre-set misra.json with a path because its different for each developers machine.
I suppose I could create a CMake step that builds a cusom misra.json but that seems cumbersome and I would not be able to keep it under source control.
To test the --addon=misra.json I hardcoded the path for my machine.
There is slightly different in output between the 2 methods.
Via: --addon=
Via: --dump
The --addon output is missing the Severity and the Rule.
Lasty there is a mismatch between the rule format you provided and what the rule paser actually accepts.
You specified:
But the parser expects rules in the following format:
misra_6.0
misra_7_0
misra.21.11
If the format you provided is to be the offical format then the regex needs updating.
Something like this should work
Yes. The output will now be formatted the same way as normal cppcheck warnings. You can use --template to get the output you want.
But yes you are right that we need to append "(Required)" in the --cli output somehow. I'd say it would be best if it was a separate item in the json output. The misra addon is not allowed to change the texts in any way.
So well.. that will lead to some hacks in cli/gui.
I think the new format should be official. But well do we need to update the parser? It just means you can't reuse your rules file as is. You will need to make simple edits.
The path to the suppression liste is ok. Its the path to the --rule-texts= that I need to set.
I'm not sure I follow yet. I'll play with the template output to better understand the limitation.
I'm talking about the format in my suppressons file not the rules-text file. An example is:
MISRA_13.4:sensors.c:110
If I change that to:
MISRA-c2012_13.4:sensors.c:110
Then the supression no longer works.
The supressions file is under source control so an update would need to be able to support both formats so it works on current and older revisions of the file.