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

Visual Studio 2017 Update 9 (15.9) Support #522

Closed
shartte opened this issue Nov 26, 2018 · 5 comments
Closed

Visual Studio 2017 Update 9 (15.9) Support #522

shartte opened this issue Nov 26, 2018 · 5 comments
Assignees
Labels
tools Issue/feature request related to tools windows Issue/feature request for Windows only

Comments

@shartte
Copy link
Contributor

shartte commented Nov 26, 2018

Describe the bug
Compilation fails when updating to the latest VS update (2017.9).
Visual Studio now requires Clang 7.
I am compiling for Windows x64.

To Reproduce
Install the latest update to Visual Studio and try to compile.
First you'll note that the MS STL now requires Clang 7.
After updating to Clang 7, the following errors occur.

Additional context

SPIRV
Three identical compile errors occurs in SPIRV:

D:\filament\third_party\glslang\SPIRV\GlslangToSpv.cpp(5469,49):  error: non-constant-expression cannot be narrowed from type 'spv::GroupOperation' to 'unsigned int' in initializer list [-Wc++11-narrowing]
            spv::IdImmediate groupOp = { false, groupOperation };
                                                ^~~~~~~~~~~~~~
D:\filament\third_party\glslang\SPIRV\GlslangToSpv.cpp(5469,49):  note: insert an explicit cast to silence this issue
            spv::IdImmediate groupOp = { false, groupOperation };
                                                ^~~~~~~~~~~~~~
                                                static_cast<unsigned int>( )

The error can be fixed by inserting the cast, but I am unsure it's correct.

Allocator

In file included from D:\filament\libs\utils\test\test_Allocators.cpp:25:
D:\filament\libs\utils\include\utils/Allocator.h(685,32):  error: no matching member function for call to 'alloc'
        return mArena.template alloc(size, alignment, 0);
               ~~~~~~~~~~~~~~~~^~~~~
D:\filament\libs\utils\test\test_Allocators.cpp(260,17):  note: in instantiation of member function 'utils::ArenaScope<utils::Arena<utils::LinearAllocator, utils::LockingPolicy::NoLock, utils::TrackingPolicy::Untracked> >::allocate' requested here
        p = ssa.allocate(1024);
                ^
D:\filament\libs\utils\include\utils/Allocator.h(533,8):  note: candidate template ignored: couldn't infer template argument 'T'
    T* alloc(size_t count, size_t alignment = alignof(T), size_t extra = 0) noexcept {
       ^

I "fix" this one by explicitly calling template alloc with <uint8_t> in the void* case like this:

    void* allocate(size_t size, size_t alignment = 1) noexcept {
        return mArena.template alloc<uint8_t>(size, alignment, 0);
    }

When I tried alloc<void>, clang cited that void is not trivially destructible.

This relates to #516 and #358

@romainguy
Copy link
Collaborator

Assigning to @pixelflinger for the Allocator issue.

@shartte The casts look correct but this would require maintaining a patch on top of glslang. You should also file a bug for glslang.

@romainguy romainguy added tools Issue/feature request related to tools windows Issue/feature request for Windows only labels Nov 26, 2018
@shartte
Copy link
Contributor Author

shartte commented Nov 26, 2018

@romainguy Yeah I'll look into that. My gut says that the underlying type for that enum should be uint32_t to begin with, but I haven't looked into it yet.

edit, Done: KhronosGroup/glslang#1596
And yeah, I'd guess a better solution than inserting the cast would be setting the underlying enum type properly.

@shartte
Copy link
Contributor Author

shartte commented Nov 26, 2018

@romainguy So I checked with the upstream master, and that compiles without issues.
After updating glslang in the filament tree to their latest master, it also compiles in filament without further changes.

@romainguy
Copy link
Collaborator

Thanks for checking, we can just move to a newer glslang then.

@prideout or @bejado would you mind updating our glslang please?

@romainguy romainguy assigned bejado and unassigned pixelflinger Nov 26, 2018
@shartte
Copy link
Contributor Author

shartte commented Nov 29, 2018

Now that #526 is merged, this issue is fixed.

@shartte shartte closed this as completed Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issue/feature request related to tools windows Issue/feature request for Windows only
Projects
None yet
Development

No branches or pull requests

4 participants