-
Notifications
You must be signed in to change notification settings - Fork 32
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
Add classes to uni/lib that implement fixed point arithmetic. #363
Conversation
I'm happy with this - looks good. The bug report in neg() is interesting in that I would have expected the original code to either error both in and out of a package or work in both cases. I take it the expression in a static assignment is executed at a different environment or point in time from the code in an initial clause. Is there a good reason for that? I think I've always thought of them as equivalent. |
I've reported it to "the authorities". I've left the workaround(s) in because we can't be sure that everyone will have the updated compiler (when it arrives). In passing, I think the dependencies in the class source code on the package name is particularly ugly but we are stuck with how packages are implemented I guess. |
FxPtTest_OVLD: FxPtTest_OVLD.icn tester.u | ||
|
||
tester.u: tester.icn | ||
$(UC) -s -u -c $^ |
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.
The test is failing because UC
relative path is wrong from this directory.
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.
But since it is also wrong for every other test in the test suite -- I guessed that the problem is in the Debian-Pkg configuration, not this PR.
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.
Yes, that is a separate issue, but as you can see, the tests above ignore. And only your PR fails the tests. Only the new Makefile attempts to compile the test in the subfolder.
|
||
include ../Makedefs | ||
|
||
ifneq ("$(wildcard $(UC))","") |
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.
Let us not mask this error. I opened a PR to fix the issue.
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.
To clarify, the debian package test , builds a debian package and installs it. The unicon used should be the installed version, and that is determined by setting the UC variable in the test environment. That was correct in once place, but it looks like another instance was added at some point without taking the environment variable into account.
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.
I'm genuinely at a loss here. I said "The problem is in the Debian-Pkg". You said "but your test fails and the others don't" (and didn't merge the PR). So I fixed the test to not fail. Then you said "let's not mask the error" and fixed the Debian-Pkg problem (and still didn't merge this PR). I could guess what modifications are needed (if any) in order to merge this PR, or you could say explicitly: I very much prefer the latter. Just to be clear, after PR367 is merged, either this version of PR363 or the previous version will both work -- which one do you want? If something else, then please say what the something else is.
As far as I can see, the other tests also use a variant of the technique I used to prevent the failure. If there's something wrong with it then there's also something wrong with the other tests (although they are testing for a feature, the skip is also selected if Unicon is absent).
I've merged PR367, rebased this commit on top of PR367 and force pushed it.
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.
Apologies for the confusion. It was obvious there is an issue in the debian pkg platform, not funding the unicon binaries. But all other tests were passing. There is no explicit "skip if we don't have Unicon", so I figured we handle that case globally and that there was still a separate issue in the PR. When you added the explicit step and it worked, I decided to go fix the platform issue.
To be clear, we don't want to have explicit skips if unicon doesn't exist, failing in that case is fine. If we get to the point where we don't have unicon binaries then we have a more fundamental issue in the test.
If operator overloading is configured the classes will support it. Otherwise, the alternative methods (add, sub etc.) can be used in place of the infix operators. Test code is in tests/unicon. There are two sets of tests, one with overloading and one without (one is a superset of the other). Unidoc documentation is embedded in the source code. Review No Compiler, No test squash
Add classes to uni/lib that implement fixed point arithmetic.
If operator overloading is configured the classes will support it. Otherwise, the alternative methods (add, sub etc.) can be used in place of the infix operators. Test code is in tests/unicon. There are two sets of tests, one with overloading and one without (one is a superset of the other).
Unidoc documentation is embedded in the source code.