Skip to content

Conversation

@hvitved
Copy link
Contributor

@hvitved hvitved commented Nov 7, 2018

This PR adds support for assertions and custom null checks in the guards library (first 4 commits). Additionally, the PR fixes the guards implications logic (last commit), as dominance is important for correctness (as discussed offline with @aschackmull).

@calumgrant

@hvitved hvitved added the C# label Nov 7, 2018
@hvitved hvitved requested a review from calumgrant November 7, 2018 14:33
@hvitved hvitved requested a review from a team as a code owner November 7, 2018 14:33
@hvitved hvitved force-pushed the csharp/more-guards branch from ae2f096 to 29f163f Compare November 8, 2018 19:22
Copy link
Contributor

@calumgrant calumgrant left a comment

Choose a reason for hiding this comment

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

Very nice indeed. I only had one comment as I read past. Do you have any performance measurements for this. Happy to merge as-is.

* Holds if assertion `a` directly asserts that expression `e` evaluates to
* value `v`.
*/
predicate asserts(Assertion a, Expr e, AbstractValue v) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Drop parameter e here as it's not adding anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used by the predicates that call asserts, so I would like to keep it in.

@hvitved
Copy link
Contributor Author

hvitved commented Nov 15, 2018

Do you have any performance measurements for this

I tested performance on my usual set of projects, and no performance issues found.

@semmle-qlci semmle-qlci merged commit 536f3f3 into github:master Nov 15, 2018
@hvitved hvitved deleted the csharp/more-guards branch November 15, 2018 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants