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

Use C++ containers for builtin versioning #3395

Merged
merged 1 commit into from
Dec 22, 2023

Conversation

ncesario-lunarg
Copy link
Collaborator

Removes some of the pointers/"end markes" used in the BuiltInFuntion versioning, replacing them with std::arrays and spans.

NOTE: The span class used is a copy of the span class that has been in use in the Vulkan-ValidationLayers as a temporary solution until C++20 is available.

NOTE: The std::arrays could be constexprs, but this requires some extra work pre-C++20, and is therefore not included in this change, but could be done in a follow up PR.

Removes some of the pointers/"end markes" used in the BuiltInFuntion
versioning, replacing them with std::arrays and spans.

NOTE: The span class used is a copy of the span class that has been in
use in the Vulkan-ValidationLayers as a temporary solution until C++20
is available.

NOTE: The std::arrays could be constexprs, but this requires some extra
work pre-C++20, and is therefore not included in this change, but could
be done in a follow up PR.
Copy link
Contributor

@arcady-lunarg arcady-lunarg left a comment

Choose a reason for hiding this comment

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

Thinking about this some more, I think using this custom span class is fine, and we should consider using it elsewhere in the codebase. If and when we upgrade to C++20 as the standard, we can delete our custom implementation.

@arcady-lunarg arcady-lunarg merged commit 0ae8960 into KhronosGroup:main Dec 22, 2023
22 checks passed
@ncesario-lunarg ncesario-lunarg deleted the update-versioning branch December 22, 2023 23:19
@1480c1
Copy link
Contributor

1480c1 commented Dec 24, 2023

This commit causes build errors with mingw-w64's g++,

FAILED: glslang/CMakeFiles/MachineIndependent.dir/MachineIndependent/Initialize.cpp.obj 
D:\gcc\msys64\mingw64\lib\ccache\bin\g++.exe -DENABLE_HLSL -DENABLE_OPT=1 -DGLSLANG_OSINCLUDE_WIN32 -ID:/gcc/build/glslang-git/build-64bit/include -D_FORTIFY_SOURCE=2 -fstack-protector-strong -mtune=generic -O2 -pipe -D__USE_MINGW_ANSI_STDIO=1 -O3 -DNDEBUG -std=c++17 -Wall -Wmaybe-uninitialized -Wuninitialized -Wunused -Wunused-local-typedefs -Wunused-parameter -Wunused-value -Wunused-variable -Wunused-but-set-parameter -Wunused-but-set-variable -fno-exceptions -fno-rtti -Werror=deprecated-copy -MD -MT glslang/CMakeFiles/MachineIndependent.dir/MachineIndependent/Initialize.cpp.obj -MF glslang\CMakeFiles\MachineIndependent.dir\MachineIndependent\Initialize.cpp.obj.d -o glslang/CMakeFiles/MachineIndependent.dir/MachineIndependent/Initialize.cpp.obj -c D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.cpp
D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.cpp:145:47: error: class template argument deduction failed:
  145 |                                               };
      |                                               ^
D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.cpp:145:47: error: no matching function for call to 'array(glslang::{anonymous}::Versioning, glslang::{anonymous}::Versioning)'
In file included from D:/gcc/msys64/mingw64/include/c++/13.2.0/bits/memory_resource.h:47,
                 from D:/gcc/msys64/mingw64/include/c++/13.2.0/list:73,
                 from D:/gcc/build/glslang-git/glslang/Include/Common.h:50,
                 from D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.h:41,
                 from D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.cpp:54:
D:/gcc/msys64/mingw64/include/c++/13.2.0/tuple:2005:45: note: candidate: 'template<class _Tp, long long unsigned int _Nm> array()-> std::array<_Tp, _Nm>'
 2005 |   template<typename _Tp, size_t _Nm> struct array;
      |                                             ^~~~~
D:/gcc/msys64/mingw64/include/c++/13.2.0/tuple:2005:45: note:   template argument deduction/substitution failed:
D:/gcc/build/glslang-git/glslang/MachineIndependent/Initialize.cpp:145:47: note:   candidate expects 0 arguments, 2 provided
  145 |                                               };
      |                                               ^

A fix is to include <array> in the header at the top, so

diff --git a/glslang/MachineIndependent/Initialize.cpp b/glslang/MachineIndependent/Initialize.cpp
index 8632d6d6..39f5a243 100755
--- a/glslang/MachineIndependent/Initialize.cpp
+++ b/glslang/MachineIndependent/Initialize.cpp
@@ -51,6 +51,7 @@
 //                                                including identifying what extensions are needed if a version does not allow a symbol
 //

+#include <array>
 #include "Initialize.h"
 #include "span.h"

would fix the issue

@ncesario-lunarg
Copy link
Collaborator Author

Thanks for the heads up @1480c1. There's a fix here, but I don't have MinGW locally and we do not support mingw in CI, so I have no way of verifying whether or not that PR works.

@1480c1
Copy link
Contributor

1480c1 commented Dec 24, 2023

Thanks

@1480c1
Copy link
Contributor

1480c1 commented Dec 24, 2023

@ncesario-lunarg as a separate info related to that, it seems someone else commented on the commit 0ae8960#commitcomment-135633905, but using what I presume to be not windows

@ncesario-lunarg
Copy link
Collaborator Author

👍 Looks like that is maybe on Arch, but is likely the same problem. I do have an Arch setup, but probably won't be able to test that until after the holidays. Hopefully the PR I put up fixes both issues.

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