Skip to content

Conversation

GregTheMadMonk
Copy link

@GregTheMadMonk GregTheMadMonk commented May 25, 2024

Also newer compilers give a warning (that gets promoted to a compile-time error) on interp.c:216. Fixed it with an explicit conversion

Admittedly, using a std::unique_ptr is a bit overkill here because CreateFrameBuffer is only called once in the entire code base, but I've decided to stay future-proof just in case.

Also, maybe project's CMAKE_CXX_STANDARD could be bumped to at least 14-17? I see no reason to keep it as low as 11, but ofc there's also an argument of "if it ain't broke don't fix it"...

@Interrupt
Copy link
Owner

Sorry for the delay. I'm a bit hesitant to create a unique_ptr here in this one case as it's not used anywhere else in the code base and like for the C++ version bump, I'd like to keep things so that it could be potentially compiled on a potato if needed.

@GregTheMadMonk
Copy link
Author

No problem

I'm a bit hesitant to create a unique_ptr here in this one case as it's not used anywhere else in the code base

Well, std::unique_ptr is a C++11 feature, which this project targets, and exists specifically for this purpose of having a unique resource that must be automatically freed. Of course, a custom class with RAII can be written for this (or all of this can even be handled by hand), but why do that if the required facilities are present in the standard library.

for the C++ version bump, I'd like to keep things so that it could be potentially compiled on a potato if needed.

Your choice, I have no info on the portability of different C++ versions to either object or support that :)

@Interrupt
Copy link
Owner

I might be being too strict regarding the usage of C++ standard library stuff - I've been wary of sending this project down a "let's port System Shock to modern C++!" rabbit hole of refactoring for refactoring's sake but for something like this it does make sense. I'll merge this in if you can resolve the conflicts.

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.

2 participants