stan-dev / stanc3

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

Fix two issues with tuple functions #1356

Closed WardBrian closed 1 year ago

WardBrian commented 1 year ago
  1. Accidentally returning a reference
  2. Integer arguments failed under certain circumstances, are now templated

These were found because the Stan tests were raising a -Wreturn-stack-address warning. We should try to catch those types of things in stanc going forward.

Submission Checklist

Release notes

Fixed a few issues with the C++ generated for functions which accept tuples as arguments.

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)

codecov[bot] commented 1 year ago

Codecov Report

Merging #1356 (45e4b82) into master (743d0dd) will increase coverage by 0.04%. Report is 2 commits behind head on master. The diff coverage is 97.89%.

:exclamation: Current head 45e4b82 differs from pull request most recent head ca286a2. Consider uploading reports for the commit ca286a2 to get more accurate results

@@            Coverage Diff             @@
##           master    #1356      +/-   ##
==========================================
+ Coverage   89.39%   89.44%   +0.04%     
==========================================
  Files          65       65              
  Lines       10607    10644      +37     
==========================================
+ Hits         9482     9520      +38     
+ Misses       1125     1124       -1     
Files Changed Coverage Δ
src/stan_math_backend/Cpp.ml 86.18% <90.90%> (+0.21%) :arrow_up:
src/stan_math_backend/Lower_functions.ml 98.80% <98.21%> (+0.13%) :arrow_up:
src/analysis_and_optimization/Memory_patterns.ml 90.57% <100.00%> (ø)
src/frontend/Ast_to_Mir.ml 94.25% <100.00%> (+0.07%) :arrow_up:
src/stan_math_backend/Lower_expr.ml 92.85% <100.00%> (ø)
src/stan_math_backend/Lower_program.ml 99.18% <100.00%> (ø)

... and 1 file with indirect coverage changes

WardBrian commented 1 year ago

IMO this will justify a 2.33.1, but the nice thing is the only thing that would be different between the two versions is the Stanc executables.

@rok-cesnovar @serban-nicusor-toptal

rok-cesnovar commented 1 year ago

Great catch. Do we wait for a few days or just do the patch release?

WardBrian commented 1 year ago

@SteveBronder and I still want to take a slightly deeper look at this, so it might be nice to also see if anything else comes in in the next couple days

WardBrian commented 1 year ago

Ok I think this is finally ready for review @SteveBronder

It's a bit tricky but honestly I think I like the code better now than before.

WardBrian commented 1 year ago

@rok-cesnovar @serban-nicusor-toptal I think we should try to do 2.33.1 in the middle of next week, giving a few more days for new issues to come in.

At the moment it would just be the 2.33.0 branches from all the repos except for stanc3