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

Re-apply cfile-update with fix regression #91

Merged
merged 4 commits into from
Apr 23, 2024

Conversation

winterheart
Copy link
Collaborator

After merging #76 there was regression found in #88. This PR re-applies changes with additional fix.

@winterheart winterheart requested a review from Arcnor April 19, 2024 15:01
@winterheart winterheart marked this pull request as draft April 19, 2024 16:46
CMakeLists.txt Outdated Show resolved Hide resolved
@winterheart
Copy link
Collaborator Author

Here additional regressions I've found in 32bit mode...

@winterheart winterheart force-pushed the cfile-update-v2 branch 2 times, most recently from d27f8c8 to 143db92 Compare April 20, 2024 06:36
@winterheart winterheart marked this pull request as ready for review April 20, 2024 06:41
@Lgt2x Lgt2x self-assigned this Apr 20, 2024
@winterheart
Copy link
Collaborator Author

PR ready to merge, though additional testing on Windows would be useful.

@Lgt2x Lgt2x assigned Arcnor and unassigned Lgt2x Apr 20, 2024
@winterheart winterheart force-pushed the cfile-update-v2 branch 3 times, most recently from fdaf346 to d6c0901 Compare April 21, 2024 15:40
CMakeLists.txt Outdated Show resolved Hide resolved
cfile/CMakeLists.txt Outdated Show resolved Hide resolved
@Lgt2x Lgt2x mentioned this pull request Apr 21, 2024
@winterheart winterheart force-pushed the cfile-update-v2 branch 2 times, most recently from f16be9d to 7c59df0 Compare April 22, 2024 18:46
Copy link
Collaborator

@Arcnor Arcnor left a comment

Choose a reason for hiding this comment

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

I think this is ready, I just think the change to use PRIVATE on the target_link_libraries is good to maintain consistency.

2dlib/CMakeLists.txt Show resolved Hide resolved
cfile/hogfile.cpp Show resolved Hide resolved
cfgetc returns int on success read, but if cast it to int8_t too early, 255 will become -1, or EOF.
Whoah, big surprise float_t and double_t may have different length. Still waiting float32_t and float64_t types from C++23...
Other modules that depends on it, can reuse includes on linking.
There some files formally not belonging any packages (lib directory), as workaround there temporary include_directories(cfile) on root of project. After migrating all modules this can be removed.
@Arcnor Arcnor merged commit 15ac86d into DescentDevelopers:main Apr 23, 2024
5 checks passed
JeodC pushed a commit that referenced this pull request Apr 28, 2024
Re-apply cfile-update with fix regression
@winterheart winterheart deleted the cfile-update-v2 branch May 27, 2024 21:57
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

3 participants