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

Add _ROR & _ROL support. Fix issues with _SHR & _SHL #153

Merged
merged 8 commits into from
Sep 3, 2022

Conversation

a740g
Copy link
Contributor

@a740g a740g commented Sep 1, 2022

@SteveMcNeill @mkilgore @RhoSigma-QB64

Opening a PR for _ROL and _ROL functions for QBPE-64.
These are implemented using multiple C functions on the LibQB side. However, they are mapped to _ROL and _ROL on the BASIC side.

@mkilgore You had mentioned an issue with _SHL where shifting bits left beyond say a byte would spill those to the word side.

I've implemented _SHR & _SHL just like _ROL and _ROL now and the issue above is now fixed.

Update: All _SHL & _SHR specific changes have been removed from this PR as some critical things will need to be considered and behaviors defined.

@a740g a740g requested review from mkilgore, QB64Cobalt, RhoSigma-QB64 and SteveMcNeill and removed request for QB64Cobalt September 1, 2022 01:38
Copy link
Contributor

@mkilgore mkilgore left a comment

Choose a reason for hiding this comment

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

IMO it would be good to add a bunch of test cases of the expected results of these functions, it would give us a good idea of how they behave (especially with type conversions) and also highlight any potentially incompatibilities with the existing _SHR and _SHL. These should be pretty easy to do, just create a ./tests/compile_tests/shifts folder and inside create a test.bas and test.output. In test.bas you can do a bunch of tests like this:

$CONSOLE:ONLY

Dim b as _Byte
DIm i as _Unsigned Integer
b = -2
i = &HAA00

PRINT _SHR(b, 2); _SHR(b, 8); _SHR(b, 20); _SHR(b, 70); _SHR(b, -1)

FOR k = -100 to 100
    PRINT _SHL(i, k); _SHR(i, k)
NEXT k

' More tests, you get the idea

And then test.output should just contain the expected console output of the program:

10 20 30 40 50
1 2
2 3
3 4
...

Obviously those numbers aren't right, but I hope it makes enough sense :) I'd be happy to help you get stuff in place if you want. For checking it locally you can just compile your test program and run it. Additionally you could make another ./tests/compile_tests/rotations for testing _ROL and _ROR.

Ideally the _SHR and _SHL tests produce the same output on the current QB64 as they do on the fixed version, but I suspect that won't be the case. The tests would help us understand the differences.

@a740g a740g requested a review from mkilgore September 1, 2022 12:07
Copy link
Member

@RhoSigma-QB64 RhoSigma-QB64 left a comment

Choose a reason for hiding this comment

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

In subs_functions.bas the names assigned to id.n = should reflect the correct "CaMeL" case, as these are used to determine the correct casing if "Camel Case" is set in IDE>Options>CodeLayout, hence _RoL and _RoR or _Rol and _Ror.

In the same run _SHL and _SHR should be corrected and all other which are not normalized for CaMeL case.

@RhoSigma-QB64 RhoSigma-QB64 added the enhancement New feature or request label Sep 2, 2022
@RhoSigma-QB64 RhoSigma-QB64 linked an issue Sep 2, 2022 that may be closed by this pull request
@mkilgore mkilgore merged commit d776f7a into QB64-Phoenix-Edition:main Sep 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please add ROR and ROL support
3 participants