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

Fixing PPM export and windows build errors with VS 22 #6946

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dabeschte
Copy link

This small PR implements all fixes that I needed to make this project compile under Windows 10 with Visual Studio 2022.

  • added vulkan: fixed missing "bluevk" library
  • set c++ standard to 20. Did not compile with 17, and defaulted at least on my system to latest experimental which did not compile.
  • fixed PPM export writing additional bytes which break the file format. This happened, because the file was not opened explicitly as a binary

(reupload because of some CLA/email conflicts)

CMakeLists.txt Outdated
@@ -166,6 +166,7 @@ if (WIN32)
# "VCRUNTIME140.dll not found. Try reinstalling the app.", but give users
# a choice to opt for the shared runtime if they want.
option(USE_STATIC_CRT "Link against the static runtime libraries." ON)
set(CMAKE_CXX_STANDARD 20)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What doesn't compile with C++17 for you?

Copy link
Author

@dabeschte dabeschte Jul 11, 2023

Choose a reason for hiding this comment

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

I get error C7555 in DriverEnums.h line 1109 and 1113

use of designated initializers requires at least '/std:c++20'

This also occurs in other files like here or here or here

Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately we can't yet support C++ 20, so we need to find another workaround for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We can fix these two lines in DriverEnums.h easily so the don't use designated initializers.

Regardless, I thing this is a problem is other places and I thought we were compiling with C++20 already on windows, specifically because of that?

Copy link
Member

Choose a reason for hiding this comment

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

My previous comment only meant that we shouldn't use C++20 globally across the project, since 1P clients are limited to C++1 7 at the moment. On Windows we have more flexibility.

On Windows, we're currently using /std:c++latest for MSVC, but maybe we need to explicitly use /std:c++20 (apparently those two flags have slightly different meanings). @dabeschte can you try replacing this line with /std:c++20 and seeing if it fixes the designated initializer error?

set(CXX_STANDARD "/std:c++17")

Copy link
Author

Choose a reason for hiding this comment

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

No that does not work, since the if (MSVC) 3 lines below overrides that. This results in MSVC using the latest preview, not the latest stable compiler version.
image

The error that I get in this case is

Error	C3878	syntax error: unexpected token 'this' following 'expression'	backend	<filament>\libs\utils\include\utils\Range.h	37	

Caused by this piece of code that actually looks fine to me, but since we are using preview features, I assume the compiler might have a bug here?

    bool overlaps(const Range<T>& that) const noexcept {
        return that.first < this->last && that.last > this->first;
    }

Anyways, changing line 289 in CMakeLists.txt to "/std:c++20" solves the problem for Windows.
That said, MSVC is actually right about the invalid usage of C++20 features in multiple files - not only in DriverEnums.h
G++ just doesn't show a warning by default

Copy link
Collaborator

Choose a reason for hiding this comment

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

C3878 has just been fixed.

@romainguy romainguy requested a review from bejado July 10, 2023 15:58
@romainguy romainguy added the internal Issue/PR does not affect clients label Sep 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal Issue/PR does not affect clients
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants