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

Dfp 080 #496

Merged
merged 6 commits into from
Aug 20, 2022
Merged

Dfp 080 #496

merged 6 commits into from
Aug 20, 2022

Conversation

JamesWekel
Copy link
Contributor

Fish,

Here is my attempt at the Decimal Floating Point Packed Conversion facility (080) instructions. Thank you for all the documentation on making Hyperion and adding new files to the solution. I used your recent BEAR-Enhancement Facility commit as an outline so I hope that I've make all the correct additions/changes.

I still have lot of rust on my skillset but this was an interesting challenge to dig into such a large solution. So, make any changes as I'm sure that I've missed something.

Thanks,
Jim

Copy link
Member

@Fish-Git Fish-Git left a comment

Choose a reason for hiding this comment

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

Hi Jim!

I've reviewed your changes closely, and while I think you did a fantastic job (I'm actually quite impressed!), I'm afraid I'm going to have to reject this PR for the following reasons:

  1. Your code fails to compile on Windows using the version of Microsoft's Visual C/C++ compiler that I'm still using. I did not bother to check whether it compiled cleanly or not using the latest version of Visual Studio, since, even if it did, it's still not standard 'C' code (unless the standards changed recently, and even if they had, Hercules still needs to support older compilers).

    The problem is lines 1561 and 1565 of dfp.c: you're using what appears to be a GCC extension that was expressly rejected by the C99 standards committee.

  2. Your "dfp-080-to-packed" (CPDT, CPXT) runtest verification test programs fails to verify. I'm getting "Test "dfp-080-to-packed.tst: CPDT, CPXT": 72 OK compares. 320 failures." Note that I tried both 0xF : 0xC and 0x0F : 0x0C for #1 above, but the test still fails the same way:

If you can correct the above two issues I would be very happy to accept your Pull Request and merge it into our repository!
 

@JamesWekel
Copy link
Contributor Author

Fish,

Thank you for the review and finding issues for me to fix!

  1. I have replaced the "0b1111 : 0b1100' with " 0x0F : 0x0C" to conform to the C99 standards. VS 2022 msvc accepts 0b
    constants and I'll try to avoid them in the future.

  2. The dfp-080-to-packed.tst was missing "$(testpath)/" so nothing to test!

Hoping that try #2 works.

Thanks again,
Jim

Copy link
Member

@Fish-Git Fish-Git left a comment

Choose a reason for hiding this comment

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

Looks good to me!

Test "dfp-080-from-packed.tst: CDPT, CXPT": 162 OK compares. All pass.
Test "dfp-080-to-packed.tst: CPDT, CPXT": 393 OK compares. All pass.
Did 2 tests. All OK.

Will merge ASAP!

THANKS!   :))

@Fish-Git Fish-Git merged commit 2bf57bf into SDL-Hercules-390:develop Aug 20, 2022
@wrljet
Copy link
Member

wrljet commented Aug 22, 2022

Very nice work on this. Thank you for your contribution.

Bill

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.

3 participants