stan-dev / stanc3

The Stan transpiler (from Stan to C++ and beyond).
BSD 3-Clause "New" or "Revised" License
138 stars 44 forks source link

Use pragmas to disable unused variable warnings in generated C++ #1422

Open WardBrian opened 1 month ago

WardBrian commented 1 month ago

I've recently been compiling a bunch of Stan models with -Wall and noticed this will often lead to a lot of unused variable warnings. We've handled most of the ones of compiler-generated variables already, mostly using the (void) variable_name; trick, but if the user's code features variables which are unused (or unused in certain contexts, like a transformed parameter which is used in generated quantities but not model), this leads to warnings during C++ compilation.

This PR explores an alternative which is to generate #pragma diagnostic ignore directives for unused variable warnings, so they will be disabled in the model's code regardless of the global warning setting.

This has the advantage of not requiring those void casts everywhere and working for users' model variables, but has the downside that it is probably not as portable, users with other compilers would see more warnings after this.

Submission Checklist

Release notes

The C++ code generated from stanc3 should provoke fewer unused variable warnings when compiled.

Copyright and Licensing

By submitting this pull request, the copyright holder is agreeing to license the submitted work under the BSD 3-clause license (https://opensource.org/licenses/BSD-3-Clause)

WardBrian commented 1 month ago

I should note another option is to generate all our variable declarations with [[maybe_unused]] in C++17, which is probably the best option once we can require that

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 95.65217% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 89.79%. Comparing base (2c42646) to head (a0ad96b). Report is 7 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1422 +/- ## ========================================== - Coverage 89.83% 89.79% -0.04% ========================================== Files 63 63 Lines 10484 10480 -4 ========================================== - Hits 9418 9411 -7 - Misses 1066 1069 +3 ``` | [Files](https://app.codecov.io/gh/stan-dev/stanc3/pull/1422?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev) | Coverage ฮ” | | |---|---|---| | [src/stan\_math\_backend/Lower\_functions.ml](https://app.codecov.io/gh/stan-dev/stanc3/pull/1422?src=pr&el=tree&filepath=src%2Fstan_math_backend%2FLower_functions.ml&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev#diff-c3JjL3N0YW5fbWF0aF9iYWNrZW5kL0xvd2VyX2Z1bmN0aW9ucy5tbA==) | `98.64% <100.00%> (-0.01%)` | :arrow_down: | | [src/stan\_math\_backend/Lower\_program.ml](https://app.codecov.io/gh/stan-dev/stanc3/pull/1422?src=pr&el=tree&filepath=src%2Fstan_math_backend%2FLower_program.ml&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev#diff-c3JjL3N0YW5fbWF0aF9iYWNrZW5kL0xvd2VyX3Byb2dyYW0ubWw=) | `99.17% <100.00%> (-0.01%)` | :arrow_down: | | [src/stan\_math\_backend/Cpp.ml](https://app.codecov.io/gh/stan-dev/stanc3/pull/1422?src=pr&el=tree&filepath=src%2Fstan_math_backend%2FCpp.ml&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=stan-dev#diff-c3JjL3N0YW5fbWF0aF9iYWNrZW5kL0NwcC5tbA==) | `88.04% <85.71%> (-0.77%)` | :arrow_down: |
andrjohns commented 1 month ago

Would this be controllable by a flag/option? CRAN rejects packages if the source has pragmas which suppress warnings

WardBrian commented 1 month ago

Would this be controllable by a flag/option? CRAN rejects packages if the source has pragmas which suppress warnings

Of course they do ๐Ÿ™ƒ Does it not matter than the pragma is not part of the R package source, but generated during the build?

Do they just do a string search, or if we put it inside the pre-existing #ifndef USING_R would that work?

andrjohns commented 1 month ago

Would this be controllable by a flag/option? CRAN rejects packages if the source has pragmas which suppress warnings

Of course they do ๐Ÿ™ƒ Does it not matter than the pragma is not part of the R package source, but generated during the build?

Nope, they run the check on the package source after the configure has been executed, so the c++ would be be there and checked (๐Ÿ˜ฎโ€๐Ÿ’จ)

Do they just do a string search, or if we put it inside the pre-existing #ifndef USING_R would that work?

Yeah just a string-search/regex, it doesn't matter if you've #ifdef-it (tried that with no success)

andrjohns commented 1 month ago

But also not a blocker, since it's trivial to add a find-and-replace for the pragma to the rstantools config

WardBrian commented 1 month ago

We could add a flag, but if rstantools is already massaging the generated C++ it might make the most sense for the change to live there

andrjohns commented 1 month ago

We could add a flag, but if rstantools is already massaging the generated C++ it might make the most sense for the change to live there

Sounds good to me, especially if it's only relevant for CRAN packages