Menu

Removal of command line arguments without a deprecation period is not very user friendly.

2019-07-18
2019-08-11
  • Richard Smith

    Richard Smith - 2019-07-18

    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.

     
  • versat

    versat - 2019-07-23

    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?

     
  • Daniel Marjamäki

    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?

     
  • versat

    versat - 2019-07-23

    There is already a PR for bringing it back: https://github.com/danmar/cppcheck/pull/2030

     
  • Richard Smith

    Richard Smith - 2019-07-23

    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.

     
  • Daniel Marjamäki

    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.

     
  • Richard Smith

    Richard Smith - 2019-07-23

    I Agree 100%. What I added was just a temporary feature because I needed it and it didn't exist.

     
  • Georgiy Komarov

    Georgiy Komarov - 2019-07-23

    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.

    I suggest we deprecate it now and say that these options will be removed in 1.95.

    Let's warn the user about deprecation, when it use one of --rule-suppression, --suppress-rules or --show-suppressed-rules with addon cli?

     
  • Richard Smith

    Richard Smith - 2019-08-05

    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.

     
  • Daniel Marjamäki

    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.

     
  • Richard Smith

    Richard Smith - 2019-08-07

    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.

     
  • Daniel Marjamäki

    I think we now have the "replacement" working.

    You can for instance suppress a warning like this:

    ./cppcheck --addon=misra --suppress=misra-c2012-18.4 file.c
    
     
  • Richard Smith

    Richard Smith - 2019-08-10

    It's close but not quite. I've found 3 items.

    This is what my misra check via --dump looks like:

    python ${MISRA_CHECK_DIR}/misra.py --quiet --rule-texts=${PROJECT_TOOLS_DIRECTORY}/cppcheck/misra-rules.txt <files>`
    

    ${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

     rule_pattern = re.compile(r'^(misra|MISRA)[_.]([0-9]+)[_.]([0-9]+)')
    

    If the format you provided is to be the offical format then the regex needs updating.

     
  • Daniel Marjamäki

    -rule-texts=${PROJECT_TOOLS_DIRECTORY}/cppcheck/misra-rules.txt

    Something like this should work

    ./cppcheck --addon=misra.json --suppression-list=${PROJECT_TOOLS_DIRECTORY}/cppcheck/misra-suppressions.txt
    

    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.

     
    • Richard Smith

      Richard Smith - 2019-08-11

      -rule-texts=${PROJECT_TOOLS_DIRECTORY}/cppcheck/misra-rules.txt

      Something like this should work

      ./cppcheck --addon=misra.json --suppression-list=${PROJECT_TOOLS_DIRECTORY}/cppcheck/misra-suppressions.txt
      

      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.

       

Log in to post a comment.

Want the latest updates on software, tech news, and AI?
Get latest updates about software, tech news, and AI from SourceForge directly in your inbox once a month.