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

Clean up __OFSUB__ usage #539

Merged
merged 2 commits into from
Jan 6, 2019
Merged

Clean up __OFSUB__ usage #539

merged 2 commits into from
Jan 6, 2019

Conversation

AJenbo
Copy link
Member

@AJenbo AJenbo commented Dec 31, 2018

This removes remaining usage of __OFSUB__, this is basically continuation of bb3ab09

This enables us to compile the following files as C:
drlg_l3.cpp
lighting.cpp
monster.cpp
stores.cpp

I have play tested things and haven't seen any negative sides to doing this.

@AJenbo AJenbo mentioned this pull request Dec 31, 2018
38 tasks
@mewmew
Copy link
Contributor

mewmew commented Jan 1, 2019

My 3am brain can't quite determine whether the translation here is correct and covers all cases (e.g handling of edge cases, int overflow, underflow). I would therefore like to refrain from merging until this has been validated. Even if it passes the it compiles test and the playable test :)

Oh, and happy new year @AJenbo!

@AJenbo
Copy link
Member Author

AJenbo commented Jan 1, 2019

Yeah get some sleep and then you can compare with #538 and #536 where I fully cleaned up some functions that makes use of __OFSUB__ :)

@AJenbo AJenbo merged commit b229a07 into diasurgical:nightly Jan 6, 2019
@AJenbo AJenbo deleted the __OFSUB__ branch January 6, 2019 02:21
@AJenbo
Copy link
Member Author

AJenbo commented Jan 6, 2019

Just as a note the reason we feel it's safe to remove __OFSUB__ is that it appears to simply be a side effect of how the compiler optimized if (a < b) into if (a - b < 0) in some cases, and as such the extra edge case protection that __OFSUB__ gives isn't needed.

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.

None yet

2 participants