5 ms·

Unused function argument is a warning any sane compiler can spit out and turn into a hard error (so this is not really the best example of what PVS studio can d

by stinos 5y ago

Unused function argument is a warning any sane compiler can spit out and turn into a hard error (so this is not really the best example of what PVS studio can do for you), which makes one wonder: why was this not caught earlier? Warnings not enabled, or ignored, and is that something which is problematic wrt something as major as OpenSSL?

misnome 5y ago
I also wonder this. "Unused parameter in function" is one of the most basic warning/linting checks. I'd be interested in finding out how this managed to reach release.
- pabs3 5y ago
  Both GCC and clang warn about unused parameters, but only when you run them with -Wextra.
  - jahnu 5y ago
    As lots of us know from experience, it's very ver hard to get an in-production project into a warning free state with warnings at maximum and warnings as errors. I'm currently working on a new project this year and it has been such a joy to have near max compiler warnings as errors with multiple compilers (and clang tidy and clang format) right from the start.
    - IanSanders 5y ago
      Warning definitions also change over time with new versions
    - stinos 5y ago
      it's very ver hard to get an in-production project True, especially for older projects which never cared about warnings to begin with. Still when I encounter one I might try it anyway, gradually if possible, to see what comes out. Not just because it is indeed just joy to have no warnings, but also because more often than not some of those warnings actually tell you there are bugs. Or for example (just had this last week) because there are projects which compile with 10k+ warnings which mean that even if you just compile a single file, it is way to hard to tell whether your new code just introduced a new warning and if so which one it is.
      - jahnu 5y ago
        Absolutely. One strategy I employed with a very old code base with a gajillion warnings was to output them to a file and compare against a reference file at the end of the build. If the output changed we would fix those and maybe any that kept happening if you added new compilation units etc. It at least helped us slowly reverse the trend of accumulating warnings.
        lelanthran 5y ago
        I did the same, but it meant that the build could not go in parallel. I also stored the warnings.txt in git so that before any commit I could simply do a git diff to see what warnings were changed by my additions.
    - AndreyKarpov 5y ago
      Integration of a static analyzer into an existing project may be hard. Especially if the project is big and old. In fact, it’s not as scary as it seems to be. There are ways to do it as easy as possible: How to introduce a static code analyzer in a legacy project and not to discourage the team - https://pvs-studio.com/en/blog/posts/0743/ https://pvs-studio.com/en/blog/posts/0743/
  - tialaramex 5y ago
    You've got to love that -Wall rusted in place as meaning not, in fact, all warnings, but instead just some arbitrary set of warnings that some people wanted many years ago and now we mustn't change it because too many projects have code that builds under -Wall -Werror only because the warnings that code triggers weren't covered by -Wall many years ago. https://xkcd.com/1172/ https://xkcd.com/1172/ in action
    - marcosdumay 5y ago
      GHC has the correct policy, that -Werror is not recommended, and if you use it there are no guarantees your build won't break on the future. Every compiler should adopt it. And every build tool should stop printing thousands of lines of "build tool passed here" on the default verbosity.
      - tialaramex 5y ago
        Another benefit to Rust editions too. In principle Rust isn't obliged to let your clearly bad program compile once the realisation is reached that it's broken. If there's a new warning emitted, and you've told the compiler not to allow warnings, well, to bad, fix the warning. However sheer weight of numbers, as with GCC, might beat that position for some future case, we can imagine that if an urgently needed warning breaks 80% of the most popular crates that's not going to be OK. We get infinite extra tries - Rust editions mean that by definition there is no Rust 2022 code today, and so we can define up front that Rust 2022 code must not have whatever egregious yet widespread problem couldn't be fixed in Rust 2018 code due to the numbers involved. When people write new code which defaults to Rust 2022 it gets flagged as bad, forcing them to fix it, but their old Rust 2018 code still compiles unless they want to go to the effort to migrate it. [Yes I know Mara is poised to ship Rust 2021, but this is a hypothetical so I used a future year instead]
  - Someone 5y ago
    Reading https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html, which says “In order to get a warning about an unused function parameter, you must either specify -Wextra -Wunused (note that -Wall implies -Wunused), or separately specify -Wunused-parameter.” So, it seems one can do -Wunused-parameter
- coldpie 5y ago
  Having unused parameters is super common, especially when using function-pointer-based APIs like qsort. Arguably they should have annotated the arguments to suppress the warnings, but that starts to get into compiler compatibility (OpenSSL is used on a _lot_ of systems with a _lot_ of compilers) and becomes a much larger question than just setting -Wno-unused-parameter.
  - commandlinefan 5y ago
    > especially when using function-pointer-based APIs like qsort Well, the function should be called somewhere, even if it's ignored when null. I can't think of a legitimate case for an unused parameter - it might be #ifdef-ed out in some cases, but it should still be referenced somewhere.
    - coldpie 5y ago
      For qsort, sure, but it's pretty common to not need the void* argument for the start_routine you pass to pthread_create, to come up with one example.
PVS-Studio 5y ago
So I want to add an explanation. As I see from the comments, many of you thought that the PVS-Studio analyzer warns about a function's unused argument. That's not quite so. Or, to be precise, this is so, but the analyzer processes this case smarter than most linters or compilers. It's a bad idea to program an analyzer so that it just issues a warning to unused arguments. Such analyzer would produce many false positives, which is why many developers don't look at (or disable) these warnings in their compilers/analyzers. The PVS-Studio analyzer implements sort of empirical "magic". PVS-Studio relies on the fact that there are arguments of the same type and some of them are not used, while the other ones are used several times. At the same time, there are a number of exceptions to the rule. For example, the diagnostic is not triggered if the number of unused arguments exceeds two. All this allows the V751 diagnostic to issue few false positives, which makes the tool surpass its competitors. To be exact, when developing PVS-Studio, we do not implement rules if we cannot make them better than those of the compilers - https://pvs-studio.com/en/blog/posts/0802/ https://pvs-studio.com/en/blog/posts/0802/ . Thanks to the diagnostic I described above, one can find interesting errors - https://pvs-studio.com/en/blog/examples/v751/ https://pvs-studio.com/en/blog/examples/v751/ . P.S. The PVS-Studio analyzer also provides a "stupid" version of this diagnostic - V2537 https://pvs-studio.com/en/docs/warnings/v2537/ https://pvs-studio.com/en/docs/warnings/v2537/ . It was developed to check code against MISRA C and MISRA C++ standards. But the case above was special and by default this diagnostic was disabled - same as the other ones related to MISRA.

by stinos 5y ago

misnome 5y ago

I also wonder this. "Unused parameter in function" is one of the most basic warning/linting checks. I'd be interested in finding out how this managed to reach release.

pabs3 5y ago
Both GCC and clang warn about unused parameters, but only when you run them with -Wextra.
- jahnu 5y ago
  As lots of us know from experience, it's very ver hard to get an in-production project into a warning free state with warnings at maximum and warnings as errors. I'm currently working on a new project this year and it has been such a joy to have near max compiler warnings as errors with multiple compilers (and clang tidy and clang format) right from the start.
  - IanSanders 5y ago
    Warning definitions also change over time with new versions
  - stinos 5y ago
    it's very ver hard to get an in-production project True, especially for older projects which never cared about warnings to begin with. Still when I encounter one I might try it anyway, gradually if possible, to see what comes out. Not just because it is indeed just joy to have no warnings, but also because more often than not some of those warnings actually tell you there are bugs. Or for example (just had this last week) because there are projects which compile with 10k+ warnings which mean that even if you just compile a single file, it is way to hard to tell whether your new code just introduced a new warning and if so which one it is.
    - jahnu 5y ago
      Absolutely. One strategy I employed with a very old code base with a gajillion warnings was to output them to a file and compare against a reference file at the end of the build. If the output changed we would fix those and maybe any that kept happening if you added new compilation units etc. It at least helped us slowly reverse the trend of accumulating warnings.
      - lelanthran 5y ago
        I did the same, but it meant that the build could not go in parallel. I also stored the warnings.txt in git so that before any commit I could simply do a git diff to see what warnings were changed by my additions.
  - AndreyKarpov 5y ago
    Integration of a static analyzer into an existing project may be hard. Especially if the project is big and old. In fact, it’s not as scary as it seems to be. There are ways to do it as easy as possible: How to introduce a static code analyzer in a legacy project and not to discourage the team - https://pvs-studio.com/en/blog/posts/0743/ https://pvs-studio.com/en/blog/posts/0743/
- tialaramex 5y ago
  You've got to love that -Wall rusted in place as meaning not, in fact, all warnings, but instead just some arbitrary set of warnings that some people wanted many years ago and now we mustn't change it because too many projects have code that builds under -Wall -Werror only because the warnings that code triggers weren't covered by -Wall many years ago. https://xkcd.com/1172/ https://xkcd.com/1172/ in action
  - marcosdumay 5y ago
    GHC has the correct policy, that -Werror is not recommended, and if you use it there are no guarantees your build won't break on the future. Every compiler should adopt it. And every build tool should stop printing thousands of lines of "build tool passed here" on the default verbosity.
    - tialaramex 5y ago
      Another benefit to Rust editions too. In principle Rust isn't obliged to let your clearly bad program compile once the realisation is reached that it's broken. If there's a new warning emitted, and you've told the compiler not to allow warnings, well, to bad, fix the warning. However sheer weight of numbers, as with GCC, might beat that position for some future case, we can imagine that if an urgently needed warning breaks 80% of the most popular crates that's not going to be OK. We get infinite extra tries - Rust editions mean that by definition there is no Rust 2022 code today, and so we can define up front that Rust 2022 code must not have whatever egregious yet widespread problem couldn't be fixed in Rust 2018 code due to the numbers involved. When people write new code which defaults to Rust 2022 it gets flagged as bad, forcing them to fix it, but their old Rust 2018 code still compiles unless they want to go to the effort to migrate it. [Yes I know Mara is poised to ship Rust 2021, but this is a hypothetical so I used a future year instead]
- Someone 5y ago
  Reading https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html, which says “In order to get a warning about an unused function parameter, you must either specify -Wextra -Wunused (note that -Wall implies -Wunused), or separately specify -Wunused-parameter.” So, it seems one can do -Wunused-parameter
coldpie 5y ago
Having unused parameters is super common, especially when using function-pointer-based APIs like qsort. Arguably they should have annotated the arguments to suppress the warnings, but that starts to get into compiler compatibility (OpenSSL is used on a _lot_ of systems with a _lot_ of compilers) and becomes a much larger question than just setting -Wno-unused-parameter.
- commandlinefan 5y ago
  > especially when using function-pointer-based APIs like qsort Well, the function should be called somewhere, even if it's ignored when null. I can't think of a legitimate case for an unused parameter - it might be #ifdef-ed out in some cases, but it should still be referenced somewhere.
  - coldpie 5y ago
    For qsort, sure, but it's pretty common to not need the void* argument for the start_routine you pass to pthread_create, to come up with one example.

PVS-Studio 5y ago

So I want to add an explanation. As I see from the comments, many of you thought that the PVS-Studio analyzer warns about a function's unused argument. That's not quite so. Or, to be precise, this is so, but the analyzer processes this case smarter than most linters or compilers. It's a bad idea to program an analyzer so that it just issues a warning to unused arguments. Such analyzer would produce many false positives, which is why many developers don't look at (or disable) these warnings in their compilers/analyzers. The PVS-Studio analyzer implements sort of empirical "magic". PVS-Studio relies on the fact that there are arguments of the same type and some of them are not used, while the other ones are used several times. At the same time, there are a number of exceptions to the rule. For example, the diagnostic is not triggered if the number of unused arguments exceeds two. All this allows the V751 diagnostic to issue few false positives, which makes the tool surpass its competitors. To be exact, when developing PVS-Studio, we do not implement rules if we cannot make them better than those of the compilers - https://pvs-studio.com/en/blog/posts/0802/ https://pvs-studio.com/en/blog/posts/0802/ . Thanks to the diagnostic I described above, one can find interesting errors - https://pvs-studio.com/en/blog/examples/v751/ https://pvs-studio.com/en/blog/examples/v751/ . P.S. The PVS-Studio analyzer also provides a "stupid" version of this diagnostic - V2537 https://pvs-studio.com/en/docs/warnings/v2537/ https://pvs-studio.com/en/docs/warnings/v2537/ . It was developed to check code against MISRA C and MISRA C++ standards. But the case above was special and by default this diagnostic was disabled - same as the other ones related to MISRA.