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.

Sign up for GitHub

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

Jump to bottom

Raise MSVC warning level from /W3 to /W4 #2100

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Raise MSVC warning level from /W3 to /W4 #2100

merged 1 commit into from
Nov 8, 2024

Conversation

kbenzie
Copy link
Contributor

@kbenzie kbenzie commented Sep 18, 2024 •
edited
Loading

This patch increases the warning level when using the MSVC compiler from /W3 to /W4 and fixes the issues found. Four warnings introduced by /W4 are disabled, all related to variable name shadowing, as they are overly prescriptive to valid code.

intel/llvm#15745

@kbenzie kbenzie requested review from a team as code owners September 18, 2024 10:10
@github-actions github-actions bot added loader Loader related feature/bug common Changes or additions to common utilities conformance Conformance test suite issues. specification Changes or additions to the specification level-zero L0 adapter specific issues command-buffer Command Buffer feature addition/changes/specification labels Sep 18, 2024
kbenzie
kbenzie commented Sep 18, 2024
View reviewed changes
source/common/ur_util.hpp Outdated Show resolved Hide resolved
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch 2 times, most recently from 4a9f277 to e8e086b Compare September 18, 2024 10:31
@RossBrunton
Copy link
Contributor

On GCC, -WExtra -WError has an issue where newer versions of the compiler might introduce new warnings (or older versions might have false positives), so you end up creating a hidden dependency on a specific version of GCC. Is this an issue with MSVC as well? And if so, is it worth having a separate CMake var to turn on -WExtra / /W4?

@kbenzie
Copy link
Contributor Author

kbenzie commented Sep 18, 2024

On GCC, -WExtra -WError has an issue where newer versions of the compiler might introduce new warnings (or older versions might have false positives), so you end up creating a hidden dependency on a specific version of GCC. Is this an issue with MSVC as well? And if so, is it worth having a separate CMake var to turn on -WExtra / /W4?

I don't think extra warnings being introduced by new compilers is actually an issue, by default warnings are not treated as errors, that's only enabled with the UR_DEVELOPER_MODE=ON option. We enable that in UR CI but not downstream.

@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch 6 times, most recently from 6a0b9b3 to 568779e Compare September 24, 2024 13:24
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch 4 times, most recently from c85cdf5 to 1c9da05 Compare October 3, 2024 14:01
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 1c9da05 to 2dd0821 Compare October 4, 2024 10:23
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 2dd0821 to 3186a14 Compare October 4, 2024 10:46
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 3186a14 to 52f8ebf Compare October 8, 2024 15:46
@kbenzie kbenzie requested review from a team as code owners October 8, 2024 15:46
@kbenzie kbenzie requested a review from jchlanda October 8, 2024 15:46
@github-actions github-actions bot added images UR images cuda CUDA adapter specific issues labels Oct 8, 2024
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 52f8ebf to 8e2f794 Compare October 8, 2024 16:09
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch 2 times, most recently from e7ee380 to 5627f44 Compare October 15, 2024 10:34
@kbenzie
Copy link
Contributor Author

kbenzie commented Oct 16, 2024

@oneapi-src/unified-runtime-cuda-write @oneapi-src/unified-runtime-level-zero-write please review

@kbenzie kbenzie mentioned this pull request Oct 17, 2024
Draft
@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Oct 21, 2024
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 5627f44 to ccbfaa2 Compare October 21, 2024 15:08
@kbenzie kbenzie removed the ready to merge Added to PR's which are ready to merge label Oct 22, 2024
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch 2 times, most recently from da462e9 to 05f2a1f Compare October 23, 2024 15:32
@kbenzie
Raise MSVC warning level from /W3 to /W4
8e56347 
This patch increases the warning level when using the MSVC compiler from
`/W3` to `/W4` and fixes the issues found. Four warnings introduced by
`/W4` are disabled, all related to variable name shadowing, as they
overly prescriptive to valid code.
@kbenzie kbenzie force-pushed the benie/windows-warning-level-4 branch from 05f2a1f to 8e56347 Compare November 7, 2024 15:38
@kbenzie kbenzie added the ready to merge Added to PR's which are ready to merge label Nov 8, 2024
@callumfare callumfare merged commit 932b399 into oneapi-src:main Nov 8, 2024
80 of 83 checks passed
Maetveis added a commit to Maetveis/unified-runtime that referenced this pull request Nov 8, 2024
@Maetveis
[CMake]: Use check_linker_flag for /DEPENDENTLOADFLAG
bd3d995 
PR oneapi-src#2100 changed the WIN32 check to check from `if(WIN32)` to
checking if link.exe is used via `CMAKE_CXX_COMPILER_LINKER_ID`.
This has two problems:
- CMAKE_CXX_COMPILER_LINKER_ID is only supported starting with CMake 3.29,
  but UR still claims to CMake versions from 3.20
  (`cmake_minimum_required` is called with this version).
  This results in the flag being silently dropped in earlier versions of
  CMake.
- There are other linkers that also support this flag for example LLD.

Using check_linker_flag resolves these issues without hard-coding a list
of known linkers.
@Maetveis Maetveis mentioned this pull request Nov 8, 2024
Open
@kbenzie kbenzie mentioned this pull request Nov 8, 2024
Merged
callumfare added a commit that referenced this pull request Nov 8, 2024
@callumfare
Merge pull request #2302 from callumfare/callum/revert_ur_2100
c94dbc8 
Revert #2100 "Raise MSVC warning level from /W3 to /W4"
@callumfare callumfare mentioned this pull request Nov 8, 2024
Open
@Maetveis Maetveis mentioned this pull request Nov 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jchlanda jchlanda jchlanda approved these changes

@cppchedy cppchedy cppchedy approved these changes

@martygrant martygrant martygrant approved these changes

@pbalcer pbalcer pbalcer approved these changes

@ProGTX ProGTX ProGTX approved these changes

@EwanC EwanC EwanC approved these changes

Assignees
No one assigned
Labels
command-buffer Command Buffer feature addition/changes/specification common Changes or additions to common utilities conformance Conformance test suite issues. cuda CUDA adapter specific issues images UR images level-zero L0 adapter specific issues loader Loader related feature/bug ready to merge Added to PR's which are ready to merge specification Changes or additions to the specification
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@kbenzie @RossBrunton @jchlanda @EwanC @ProGTX @pbalcer @martygrant @cppchedy @callumfare