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

Silent cast errors #91

Closed
wants to merge 2 commits into from

Conversation

merkste
Copy link
Contributor

@merkste merkste commented Sep 28, 2018

Currently, integer overflows and underflows are silent errors during expression evaluation. This may lead to models that are broken without a chance for the author to recognise the error.

This PR addresses issue #92 and consists of two parts:

  1. Throw an error if a double cannot be casted to int
    Two classes are touched that cast the result of some prism functions and operators to int values. There might be more but non-obvious places where this happens. The commit also gets rid of unnecessary manual boxing and outmost scope variable declarations in the touched code.

  2. Change error message for PrismLangExceptions
    This part allows to see the line and column numbers if a PrismLangException is raised, e.g., on cast errors.

Steffen Märcker added 2 commits September 28, 2018 19:16
Previously, integer overflows and underflows were silent errors
during expression evaluation. This may lead to models that are
broken without a chance for the author to recognise the error.

This commit tackles the problem by modifying the evaluation
of binary operators and functions in the spirit of java.Math.
If a PrismLangException is detected, use toString() instead of
getMessage() to ensure that the line and column numbers are included.
@merkste merkste mentioned this pull request Sep 28, 2018
@merkste
Copy link
Contributor Author

merkste commented Sep 28, 2018

To be clear: This change does affect the checkExpression and StateValues methods as they should™ not be relevant in model construction. It remains to discuss whether the problem should be addressed there as well by a separate PR.

kleinj pushed a commit that referenced this pull request Oct 17, 2018
…f Booleans, Integers and Doubles

During the evaluate calls, the code previously used 'new Boolean' and 'new
Integer' constructors for the result values. By removing these allocations
and simply letting Java take care of boxing the resulting primitive
values, the integrated caching of Java can avoid creating objects in a
lot of cases (always for Boolean, and often for Integers in a certain
range).

(adapted by Joachim Klein from #91)

Tag: performance
kleinj pushed a commit that referenced this pull request Oct 18, 2018
kleinj pushed a commit that referenced this pull request Oct 18, 2018
…r integer evaluation

For the evaluation of integer math operations, use the ...Exact methods from
java.lang.Math to catch integer under-/overflows and throw PrismLangExceptions.

(adapted by Joachim Klein from #91)

Tag: performance
kleinj pushed a commit that referenced this pull request Oct 18, 2018
(adapted by Joachim Klein from #91)

Tag: performance
@kleinj
Copy link
Member

kleinj commented Oct 18, 2018

Hi @merkste,

thanks for the PR. I've split up and tweaked the commits a bit so that the unboxing changes are in a seperate commit (for performance testing) and added overflow-aware handling for the unary minus as well; those are pushed to master. I did some cursory performance testing by building models from the prism-benchmark suite; the boxing stuff seems to have a minor positive impact, the overflow-aware math a minor negative impact (as always depending on the particular model). Overall it seems to be reasonable to include for the benefit of better checking.

For the "Change error message for PrismLangExceptions" commit, I had a look and did not see how the change has an effect, the toString() call in PrismLangException just calls getMessage() as well, so the line number etc should appear with the current code as well?

I've also started writing test cases for these integer overflows, however currently the prism-auto-based test suite lacks a mechanism for forcing a specific engine for a test case (i.e., to ignore those test cases for engines other than 'explicit') when running with extra arguments.

As you mentioned the checkExpression and StateValues integer operations, for those there is a design decision what to do if an overflow error happens in those parts of the state space that are not actually relevant (i.e., not covered by the statesOfInterest). In principle, the values there could be arbitrarily broken. Unfortunately, there does not seem a smart way to have something similar to NaN for ints...

@merkste
Copy link
Contributor Author

merkste commented Jun 15, 2020

@kleinj @davexparker : As far as i can see, this PR (expect for the PrismLangException part) is integrated in the commits

Is this correct? If yes, I suggest to close the PR.

@merkste merkste closed this Aug 25, 2020
@merkste merkste deleted the silent-cast-errors branch February 14, 2022 18:42
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

2 participants