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

[WardBrian]

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

• Run unit tests
• Documentation
  • If a user-facing facing change was made, the documentation PR is here:
  • OR, no user-facing changes were made

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]

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]

Codecov Report

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

Project coverage is 89.79%. Comparing base (5692dc4) to head (59e9154).

Files with missing lines	Patch %	Lines
src/stan_math_backend/Cpp.ml	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1422      +/-   ##
==========================================
- Coverage   89.82%   89.79%   -0.04%     
==========================================
  Files          63       63              
  Lines       10598    10594       -4     
==========================================
- Hits         9520     9513       -7     
- Misses       1078     1081       +3     

Files with missing lines	Coverage Δ
src/stan_math_backend/Lower_functions.ml	98.21% <100.00%> (-0.01%)	⬇️
src/stan_math_backend/Lower_program.ml	99.17% <100.00%> (-0.01%)	⬇️
src/stan_math_backend/Cpp.ml	88.04% <85.71%> (-0.77%)	⬇️

[andrjohns]

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

[WardBrian]

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]

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]

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

[WardBrian]

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]

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