-
-
Notifications
You must be signed in to change notification settings - Fork 314
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
Windows fixes #1302
Windows fixes #1302
Conversation
Not sure what I've done that broke the AVC box checks.... |
@fancycode Can we increase the C++ version to C++20 ? Ubuntu 20.04 uses gcc-9, which does not support full C++20 (https://gcc.gnu.org/projects/cxx-status.html), but it does support designated initializers. The safer choice would be C++17. |
"Designated initializers" are only used in one place in PR #1296 . If the step to C++20 is not completely safe for older distributions, we could also use C++17 as an intermediate step. |
@@ -50,7 +50,13 @@ | |||
#define M_PI 3.14159265358979323846 | |||
#endif | |||
|
|||
#include <unistd.h> // TODO: Windows | |||
#if !defined(_WIN32) && !defined(_WIN64) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#ifndef _WIN32
should generally be enough to detect all Windows.
Btw, unistd.h
is available for MinGW (just pulls in io.h
and adds a few more constants), but I guess you'd still want to handle the temp file creation in a consistent way for all Windows builds...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Temp file creation is currently not used. I did some experiments with it and it may be an option later on, but currently, it is never called.
Perhaps separate the C++20 changes to an independent PR? Cygwin will probably also not ship GCC 13 w/ full C++20 support well until next summer (it was just bumped to GCC 12). |
According to https://gcc.gnu.org/projects/cxx-status.html, there is only one C++20 feature that requires GCC 13 (Atomic Compare-and-Exchange with Padding Bits). I don't think that this is a blocker. |
A few more unfortunately, but maybe also irrelevant for libheif code: https://en.cppreference.com/w/cpp/compiler_support/20 (see also library features further down, i.e. https://gcc.gnu.org/onlinedocs/libstdc++/manual/status.html#status.iso.2020) |
Yes seems irrelevant. That's just calendar and text formatting in the library. |
@@ -244,7 +244,7 @@ int main(int argc, char** argv) | |||
//while ((opt = getopt(argc, argv, "q:s")) != -1) { | |||
while (true) { | |||
int option_index = 0; | |||
int c = getopt_long(argc, argv, "hq:sd:C:vo:", long_options, &option_index); | |||
int c = getopt_long(argc, argv, (char*)"hq:sd:C:vo:", long_options, &option_index); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of all these (char*)
casts, it might be cleaner to change the getopt*
implementation in the extras
directory.
I've merged most of it by cherry-picking. The I've fixed the integer check in the ftyp parsing. Let's try to set the language standard to C++20. This will also give us |
@bradh Please close if everything works for you. |
Verified it builds ok on windows, at least for the release build. We may need to update catch to a get the development build working, but I am OK for now. |
The unistd change is untested, but should at least compile.
The changes for the utilities are
const char*
incompatibility withchar *
.The C++ 20 is driven by
use of designated initializers requires at least '/std:c++20'
which we use in structure initialisation in a few places.The fallthrough will require at least C++ 17, even if we change the initialisers.