-
Notifications
You must be signed in to change notification settings - Fork 462
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
feat: improvements to test_extern command #3075
Conversation
|
test_extern.lean:7:0-7:17: error: test_extern: HAdd.hAdd does not have an @[extern] attribute | ||
test_extern.lean:9:0-9:86: error: native implementation did not agree with reference implementation! | ||
test_extern.lean:6:0-6:17: error: test_extern: HAdd.hAdd does not have an @[extern] attribute or @[implemented_by] attribute | ||
test_extern.lean:8:0-8:86: error: native implementation did not agree with reference implementation! | ||
Compare the outputs of: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the error message say to compare the outputs instead of showing what the outputs are? I think it should actually print the outputs (when possible), because these issues can be OS dependent so it's important to know what value was actually produced in CI since they may not reproduce locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good suggestion, but I'd prefer not to take it on now. Happy to either open an issue, let you do that. Or, for that matter, to accept commits on this PR, or promise to review a new PR! :-)
Another improvement I found a need for in #3961 was to allow specifying the desired result which both versions of the function should evaluate to. |
Two improvements suggested by @digama0 after the initial PR was merged.
implemented_by
attributes as well.DecidableEq
rather thanBEq
for stricter testing.