-
Notifications
You must be signed in to change notification settings - Fork 11
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
Translation from Conjure-Oxide Model to Minion Model for XYZ Problem #75
Conversation
c537a8b
to
d0b9cf5
Compare
d0b9cf5
to
ba89eae
Compare
@lixitrixi @ozgurakgun What are your thoughts on #[non_exhaustive] on public AST things? https://doc.rust-lang.org/reference/attributes/type_system.html#the-non_exhaustive-attribute For our purposes, it would effectively always force a _ => Err() branch on any pattern matching involving the ast. |
as far as I understand this forces users to always handle the _ case, is that right? it could be good for documenting intent, but on the other hand I think we want compiler warnings/errors when we add new cases instead of silently compiling without warnings. code that only cares abotu some of the cases can still use _, but code that is intented to be exhaustive will get a compile time warning/error instead. does that make sense? |
Note you can use:
Where you don't want to worry about future cases (and that could be added in commits which add a new thing we temporarily dont want to worry about) |
Thank you - I will add these explicitly so that they aren't cargo check
--fix 'ed by accident.
I think there may still be some potential uses of non_exhaustive in the
minion_rs crate - in particular, with error handling and adding new
constraints that are not implemented up the stack yet - but that is out of
the scope of this PR and would be something to think about when we get
back around to polishing up the solver crates.
…On 16/11/2023 10:55, Christopher Jefferson wrote:
Note you can use:
|#[allow(unreachable_patterns)] _ => panic!(""), |
Where you don't want to worry about future cases (and that could be
added in commits which add a new thing we temporarily dont want to worry
about)
—
Reply to this email directly, view it on GitHub
<#75 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQHNE3BSUKZOS674VHEXHLYEXWJVAVCNFSM6AAAAAA7M64UFWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJUGIYTGNBUGE>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
@ozgurakgun I have translation working for XYZ from the flat conjure to minion. Could do with more testing, but I am relying on private methods and manual construction of all the AST stuff ( i think ::new(), etc are in #32 ?), so these will need to be rewritten anyways. |
29c1724
to
69e36b9
Compare
69e36b9
to
c8e7273
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor comments, looks good to me!
c8e7273
to
070f124
Compare
070f124
to
83bdfb8
Compare
@ozgurakgun I can confirm that I can no longer merge PRs to main :) |
As per #68