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

Fix for #56 (Explicit -steadystate computation broken for initial BSCCs) #54

Merged
merged 2 commits into from
Mar 6, 2018

Conversation

merkste
Copy link
Contributor

@merkste merkste commented Mar 1, 2018

Test whether all initial states are in same BSCC is wrong.

@kleinj
Copy link
Member

kleinj commented Mar 1, 2018

Thanks. I've opened an issue #56 to document the problem.

We should probably add a test case with the fix as well.

Have not yet looked at your proposed fix.

@merkste merkste force-pushed the fix-bug-explicit-ss branch 2 times, most recently from 28a3c02 to 90cf8be Compare March 5, 2018 09:08
@kleinj kleinj mentioned this pull request Mar 5, 2018
@kleinj
Copy link
Member

kleinj commented Mar 5, 2018

To clarify the logic of the tests, it might make sense to rename the BitSet test to BitSet initNotInBSCC or something. The if ((test.cardinality() < numInit) could use some intuitive explanation in the comment as well. Perhaps something like
// not all of the init states are outside the BSCC, so they are spread over at least 2 (B)SCCs and we can abort here

Otherwise, looks good to me.

Could you please do the following:

  1. Update the commit from Variable scopes explicit ss #53 (see comment there). Then, we can merge that as part of this PR.
  2. Combine the test case commit from kleinj/prism@6aa40ae with your fix commit.
  3. Add a 'Fixes: 56' in the commit message of the fix commit.

Then, from my side, we could merge. I'll look into using SCCInfo to avoid the Bitsets later on.

@merkste merkste force-pushed the fix-bug-explicit-ss branch 3 times, most recently from 3ab3cbc to 6d69028 Compare March 5, 2018 11:23
@merkste
Copy link
Contributor Author

merkste commented Mar 5, 2018

Done. =)

@kleinj kleinj changed the title Fix bug explicit ss Fix for #56 (Explicit -steadystate computation broken for initial BSCCs) Mar 5, 2018
@kleinj kleinj self-requested a review March 5, 2018 14:44
Copy link
Member

@kleinj kleinj left a comment

Choose a reason for hiding this comment

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

Looks good to me, now.

@kleinj kleinj requested a review from davexparker March 5, 2018 14:45
Steffen Märcker added 2 commits March 5, 2018 15:49
Variables should always be declared in the innermost scope.
This improves efficiency wrt. registers and GC.
prismmodelchecker#56)

The broken logic tests whether there are is a bscc that includes a non-inital state.
Copy link
Member

@davexparker davexparker left a comment

Choose a reason for hiding this comment

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

Looks good to me too. Thanks both.

@kleinj kleinj merged commit 36e2804 into prismmodelchecker:master Mar 6, 2018
@merkste merkste deleted the fix-bug-explicit-ss branch March 6, 2018 11:34
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.

None yet

3 participants