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: Use the right >/</>=/<= comparison operators for bitvectors in JS #2716

Merged
merged 4 commits into from
Sep 20, 2022

Conversation

cpitclaudel
Copy link
Member

Closes #2672.

@cpitclaudel cpitclaudel force-pushed the cpitclaudel_fix-2672 branch 4 times, most recently from a5ef771 to d44dbe0 Compare September 8, 2022 21:51
Comment on lines +1 to +7
// RUN: %dafny /compile:0 "%s" > "%t"
// RUN: %dafny /noVerify /compile:4 /compileTarget:cs "%s" >> "%t"
// RUN: %dafny /noVerify /compile:4 /compileTarget:js "%s" >> "%t"
// RUN: %dafny /noVerify /compile:4 /compileTarget:go "%s" >> "%t"
// RUN: %dafny /noVerify /compile:4 /compileTarget:java "%s" >> "%t"
// RUN: %dafny /noVerify /compile:4 /compileTarget:py "%s" >> "%t"
// RUN: %diff "%s.expect" "%t"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use %testDafnyForEachCompiler %s instead? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you remind me how to generate the output with that flag?

Copy link
Member

@robin-aws robin-aws Sep 8, 2022

Choose a reason for hiding this comment

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

It should just be a single copy of the program's output, not including the "Dafny program verifier ..." line. So in this case just:

true true true true true true true true true
false false false false false false false false false
false false false false false false false false false
true true true true true true true true true
true true true true true true true true true
false false false false false false false false false
true true true
false false false

It occurs to me that having a flag on the TestDafny tool to generate the expect file would be super handy, let me know see if I can add that. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Unfortunately it doesn't correctly detect unsupported features, so it fails on this example; I've reported the error at #2718 , and I'm currently preparing a PR. We can update this test file once that PR is merged, but I don't think we need to block on it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure we should add the output generation to TestDafny, btw, since it's needed for all lit tests (not just for for-each-compiler). Maybe we could update runTest.py to recognize that case in addition to the usual diff-based ones.

Copy link
Member

Choose a reason for hiding this comment

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

I think both makes sense. Since LIT tests can be more complicated than just piping to a single *.expect file, runTest.py can't realistically generate the "expected output" for an arbitrary LIT test (nor would this make sense as a feature on the xUnit-based LIT test runner either). I say we update it to recognize %testDafnyForEachCompiler but have it delegate generating the output file to TestDafny.

Part of the reason I'd prefer that is I'd also find it useful to install testdafny as a local dotnet tool and run it in this mode on arbitrary source files as part of development.

callString = "isLessThan";
} else {
opString = "<";
Contract.Assert(false);
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT we always throw new cce.UnreachableException(); after such a contract assertion. That ensures control flow doesn't just continue in release builds.

robin-aws
robin-aws previously approved these changes Sep 9, 2022
@cpitclaudel cpitclaudel merged commit 968595c into master Sep 20, 2022
@cpitclaudel cpitclaudel deleted the cpitclaudel_fix-2672 branch September 20, 2022 19:26
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.

JavaScript compiler: incorrect bignum comparison
2 participants