Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up AST by specifying vardecl as sized #1203

Merged
merged 1 commit into from
May 26, 2022

Conversation

WardBrian
Copy link
Member

We currently have the type of VarDecl set up so that it could hold an unsized type. This is never done in the AST, and leads to a lot of unecessary hand-wringing, like this unreachable code in the typechecker:

  | VarDecl {decl_type= Unsized _; _} ->
      (* currently unallowed by parser *)
      Common.FatalError.fatal_error_msg
        [%message "Don't support unsized declarations yet."]

We cannot do the same in the MIR, since certain optimizations actually can produce unsized declarations, namely function inlining
#1097

Submission Checklist

  • Run unit tests
  • Documentation
    • OR, no user-facing changes were made

Release notes

Internally clean up how variable declarations are represented.

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 WardBrian requested a review from nhuurre May 25, 2022 16:19
@WardBrian WardBrian added the cleanup Code simplification or clean-up label May 25, 2022
@nhuurre
Copy link
Collaborator

nhuurre commented May 25, 2022

This cleans up the code nicely but I thought the plan has been to add unsized local variables to the language? Have you discussed this with Bob?

@WardBrian
Copy link
Member Author

@bob-carpenter and I both seem to think now that something like auto or var from Java would be better than unsized declarations, for what it is worth.

If we do decide to add unsized declarations then reverting this change is just as easy as making it, but leaving this code there "just in case" seems unnecessary to me

Copy link
Collaborator

@nhuurre nhuurre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright.

@WardBrian WardBrian merged commit a8ce0f2 into stan-dev:master May 26, 2022
@WardBrian WardBrian deleted the ast-vardecl-sized branch May 26, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Code simplification or clean-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants