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

Windows fixes #1302

Closed
wants to merge 5 commits into from
Closed

Windows fixes #1302

wants to merge 5 commits into from

Conversation

bradh
Copy link
Contributor

@bradh bradh commented Sep 8, 2024

The unistd change is untested, but should at least compile.

The changes for the utilities are const char* incompatibility with char *.

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.

@bradh
Copy link
Contributor Author

bradh commented Sep 8, 2024

Not sure what I've done that broke the AVC box checks....

@farindk
Copy link
Contributor

farindk commented Sep 9, 2024

@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.

@farindk
Copy link
Contributor

farindk commented Sep 9, 2024

"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)
Copy link
Contributor

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...

Copy link
Contributor

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.

@kmilos
Copy link
Contributor

kmilos commented Sep 9, 2024

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).

@farindk
Copy link
Contributor

farindk commented Sep 9, 2024

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.

@kmilos
Copy link
Contributor

kmilos commented Sep 9, 2024

there is only one C++20 feature that requires GCC 13

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)

@farindk
Copy link
Contributor

farindk commented Sep 9, 2024

there is only one C++20 feature that requires GCC 13

A few more unfortunately, but maybe also irrelevant for libheif code:

Yes seems irrelevant. That's just calendar and text formatting in the library.
For the compiler, even gcc-11 should be ok.

@@ -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);
Copy link
Contributor

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.

@farindk
Copy link
Contributor

farindk commented Sep 10, 2024

I've merged most of it by cherry-picking.

The (char*) casts have been removed in favor of fixing the Windows getopt* implementations.

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 std::cmp_greater and similar, which should help in reducing the number of integer casts in the safety checks during parsing. If we run into packaging problems, we may have to revert back to C++17.

farindk added a commit that referenced this pull request Sep 10, 2024
farindk added a commit that referenced this pull request Sep 10, 2024
@farindk
Copy link
Contributor

farindk commented Sep 10, 2024

@bradh Please close if everything works for you.

@bradh
Copy link
Contributor Author

bradh commented Sep 16, 2024

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.

@bradh bradh closed this Sep 16, 2024
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