Build ICD Loader using Clang on Windows #177
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Kerilk
reviewed
May 24, 2022
| @@ -270,7 +270,7 @@ void *khrIcdOsLibraryLoad(const char *libraryName) | |||
| } | |||
| if (!hTemp) | |||
| { | |||
| KHR_ICD_TRACE("Failed to load driver. Windows error code is %d.\n", GetLastError()); | |||
| KHR_ICD_TRACE("Failed to load driver. Windows error code is %lu.\n", GetLastError()); | |||
There was a problem hiding this comment.
I would use %"PRIuDW" here instead of %lu.
Suggested change
| KHR_ICD_TRACE("Failed to load driver. Windows error code is %lu.\n", GetLastError()); | |
| KHR_ICD_TRACE("Failed to load driver. Windows error code is %"PRIuDW".\n", GetLastError()); |
Kerilk
reviewed
May 24, 2022
Comment on lines
-139
to
-143
| cmake-latest: | ||
| runs-on: ${{ matrix.OS }} | ||
| strategy: | ||
| matrix: | ||
| OS : [ubuntu-20.04] |
There was a problem hiding this comment.
Ubuntu 20.04 is not tested anymore after those changes. Is this intended?
Kerilk
reviewed
May 24, 2022
Comment on lines
+285
to
+297
| #if defined(_MSC_VER) && !defined(__clang__) | ||
| #pragma warning( push ) | ||
| #pragma warning( disable : 4152 ) | ||
| #elif defined(__clang__) | ||
| #pragma clang diagnostic push | ||
| #pragma clang diagnostic ignored "-Wpedantic" | ||
| #endif | ||
| return GetProcAddress( (HMODULE)library, functionName); | ||
| #if defined(_MSC_VER) && !defined(__clang__) | ||
| #pragma warning( pop ) | ||
| #elif defined(__clang__) | ||
| #pragma clang diagnostic pop | ||
| #endif |
There was a problem hiding this comment.
I would try using:
Suggested change
| #if defined(_MSC_VER) && !defined(__clang__) | |
| #pragma warning( push ) | |
| #pragma warning( disable : 4152 ) | |
| #elif defined(__clang__) | |
| #pragma clang diagnostic push | |
| #pragma clang diagnostic ignored "-Wpedantic" | |
| #endif | |
| return GetProcAddress( (HMODULE)library, functionName); | |
| #if defined(_MSC_VER) && !defined(__clang__) | |
| #pragma warning( pop ) | |
| #elif defined(__clang__) | |
| #pragma clang diagnostic pop | |
| #endif | |
| return (void *)(intptr_t)GetProcAddress( (HMODULE)library, functionName); |
here. It would be consistent with what we do at other place in the code. You may require an additional include.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This work was born out of grievance of reproducing non-MSVC specific warnings, errors and linker failures on the OpenCL-Layers project on Windows without having to fire up containers or building-installing under WSL. (At a larger scale, using Clang-derivates on Windows such as ComputeCpp often blow up the console with warnings if the device compiler sees the OpenCL headers.)
Beside touching up the source code to fix warnings, two major things were implemented in CI:
clang-cl.exe, the MSVC-like CLI of Clang.Note: Overall I'm fairly unsatisfied how much code getting this range of CI coverage takes. (Linux/Mac/Win, GCC/Clang/MSVC, C99/11/17, with/without lang exts, Debug/Release) CI is doing the very same thing, but with 2-3 lines of difference for every test matrix elem. I'll be thinking about how to achieve the same coverage with a unified script and much less duplication of code. I would like the scripts to be much simpler.