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

some trigger updates #25

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

yizhou7
Copy link

@yizhou7 yizhou7 commented Oct 14, 2021

removed some tiggers that repeats dafny's selection
added some comments for dafny selected trigger

Copy link
Contributor

@parno parno left a comment

Choose a reason for hiding this comment

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

A few minor suggestions. I think we had also discussed updating the README or STYLE file with a note along the lines of "The {:trigger} annotations used in the standard library are an advanced topic and should not be needed when starting out on Dafny projects."

src/Collections/Sequences/Seq.dfy Outdated Show resolved Hide resolved
src/Collections/Sequences/Seq.dfy Outdated Show resolved Hide resolved
src/Collections/Sequences/Seq.dfy Outdated Show resolved Hide resolved
@parno
Copy link
Contributor

parno commented Oct 15, 2021

Cool, thanks for the changes!

Copy link
Collaborator

@RustanLeino RustanLeino left a comment

Choose a reason for hiding this comment

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

Nice! Thank you. It really good to see so many manual triggers go. I have a few specific comments below.

src/Collections/Maps/Imaps.dfy Outdated Show resolved Hide resolved
src/Collections/Maps/Imaps.dfy Show resolved Hide resolved
src/Collections/Maps/Imaps.dfy Outdated Show resolved Hide resolved
Comment on lines +36 to 37
/* Dafny selected triggers: {x in xs}, {x in m}, {x in m'} */
ensures forall x {:trigger x in m'} :: x in m' ==> x in m && x !in xs
Copy link
Collaborator

Choose a reason for hiding this comment

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

The trigger in the other direction (that is, x in m) seems equally useful to me. Is there a reason not to include it?

Copy link
Author

Choose a reason for hiding this comment

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

Since the ensures is an implication, we might only want the x in m'? My limited understanding is that we are ensuring the properties of m', rather than the original m. So might not be necessary to invoke this fact every time we see the original map.

src/Collections/Sequences/Seq.dfy Outdated Show resolved Hide resolved
src/Collections/Sets/Isets.dfy Outdated Show resolved Hide resolved
@parno
Copy link
Contributor

parno commented Dec 2, 2021

@RustanLeino I believe Yi has addressed all of the issues you raised. Is this ready to be merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants