Closed nhuurre closed 1 year ago
How would this interact with the external C++ functions feature?
I think a lot of this is good - generating forward decls for everything and removing any language from the documentation about the need for them is good. We can also make function declarations completely order-independent with this, if we’re willing to do 2 passes over the function block in typechecking, which I would like to see as part of a change like this.
That said, I think we probably want to pair this with something like an @external
annotation (see https://github.com/stan-dev/design-docs/pull/45) for their use with external C++, before any deprecation warning was emitted.
More specific review on the code to follow, those are just my thoughts based on the PR description
How would this interact with the external C++ functions feature?
Oh, yeah, I forgot to talk about that. The deprecation here only applies to functions that have a definition. Undefined functions don't emit warnings. I was thinking of requiring that external functions should be declared with
extern "C++" real foo(real);
in analogy with extern "C"
you'd have in C++.
extern "C++" real foo(real);
is an interesting suggestion. For what it's worth, I think "being similar to C++" isn't a strong reason for doing something in Stan, but historically that has been the case.
The thing I like about @extern
is the potential to do something like this:
@extern("foo.hpp")
real foo(real);
This annotation does 3 things:
I think an extern keyword could easily do 1 and 2, but I'm not sure how to do 3 with it.
Merging #1277 (a9955a7) into master (975b213) will decrease coverage by
0.08%
. The diff coverage is90.22%
.
@@ Coverage Diff @@
## master #1277 +/- ##
==========================================
- Coverage 88.81% 88.73% -0.09%
==========================================
Files 64 64
Lines 9668 9708 +40
==========================================
+ Hits 8587 8614 +27
- Misses 1081 1094 +13
Impacted Files | Coverage Δ | |
---|---|---|
src/middle/Mem_pattern.ml | 66.66% <0.00%> (ø) |
|
src/middle/UnsizedType.ml | 72.80% <33.33%> (-0.47%) |
:arrow_down: |
src/analysis_and_optimization/Optimize.ml | 91.18% <85.00%> (-0.16%) |
:arrow_down: |
src/stan_math_backend/Lower_expr.ml | 92.37% <87.50%> (-0.25%) |
:arrow_down: |
src/stan_math_backend/Transform_Mir.ml | 95.05% <87.80%> (-1.57%) |
:arrow_down: |
src/frontend/Typechecker.ml | 89.65% <91.66%> (-0.03%) |
:arrow_down: |
src/frontend/Deprecation_analysis.ml | 91.75% <94.73%> (+0.39%) |
:arrow_up: |
src/stan_math_backend/Lower_functions.ml | 97.82% <94.82%> (-2.18%) |
:arrow_down: |
src/frontend/Ast.ml | 50.24% <100.00%> (+0.24%) |
:arrow_up: |
src/frontend/Canonicalize.ml | 96.62% <100.00%> (+0.19%) |
:arrow_up: |
... and 4 more |
I'm going to hold off on a final review/approval until after the language meeting tomorrow in case anyone has some novel thoughts on extern
functions which are worth considering before this change
During the language meeting we discussed the extern
syntax. I'll be opening another issue here to discuss it more fully but long story short is it isn't a blocker for this. @bob-carpenter is very excited this change is finally being made
Submission Checklist
Closes #976
The backend simply generates a forward declaration for every function (even nonrecursive ones) which also simplifies functor struct generation a bit. Also needed to do something smarter for potentially cyclic eigen templates (#1228 heuristic has been broken for a while now because simple recursive functions don't actually need forward decls)
And also I've allowed variables to shadow user defined functions and not just stan math functions. The reason is shown here:
Release notes
Recursive user-defined functions no longer need forward declarations.
Copyright and Licensing
Copyright holder: Niko Huurre 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)