Conversation
HuaHuaY
changed the title
|
|
There was a problem hiding this comment.
Pull request overview
Enables MSVC’s conforming preprocessor for C++20 grammar support.
Changes:
- Adds
/Zc:preprocessor. - Removes the
/wd5105suppression and comments.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # * https://developercommunity.visualstudio.com/content/problem/1249671/stdc17-generates-warning-compiling-windowsh.html | ||
| set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /wd5105") | ||
| # * https://devblogs.microsoft.com/cppblog/announcing-full-support-for-a-c-c-conformant-preprocessor-in-msvc/ | ||
| set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Zc:preprocessor") |
There was a problem hiding this comment.
Yes. I also think so. Based on the reference links in the original comments, I think if we want to remove /wd5105, we should upgrade the version of windows' SDK instead of using new preprocessor.
github-actions
Bot
added
awaiting committer review
|
We can't merge this PR because boost 1.88 can't be compiled with I plan to close this PR. We can reopen it when we upgrade boost to 1.89. |
HuaHuaY
changed the title
|
I've pushed a new commit. This PR now simply removes the |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
cpp/cmake_modules/SetupCxxFlags.cmake:221
- This hunk only removes
/wd5105; it does not enable MSVC's conforming preprocessor. The MSVC branch still initializesCXX_COMMON_FLAGSas/W3 /EHscand no/Zc:preprocessorflag exists elsewhere, so the stated__VA_OPT__/C++20 preprocessing behavior is unchanged. Please add/Zc:preprocessorto the MSVC flags, while retaining or gating/wd5105as discussed in the existing review thread.
if(ARROW_USE_STATIC_CRT)
| @@ -218,23 +218,6 @@ if(WIN32) | |||
| set(CXX_COMMON_FLAGS "/W3 /EHsc") | |||
| endif() | |||
|
|
|||
|
@raulcd I think we may be able to remove |
For this reason, I close this PR for now. |
|
Follow suggestion at #50978 (comment) and reopen this PR. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
cpp/cmake_modules/SetupCxxFlags.cmake:225
- Removing
/wd5105will reintroduce C5105 warnings coming from Windows SDK headers; in configurations that enable/WXthis can become a hard build break even with/Zc:preprocessorenabled. Please keep the suppression (or gate its removal on a known-safe Windows SDK version).
# Enable the conforming preprocessor to support C++20 features.
#
# See also:
# * https://devblogs.microsoft.com/cppblog/announcing-full-support-for-a-c-c-conformant-preprocessor-in-msvc/
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Zc:preprocessor")
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
cpp/cmake_modules/SetupCxxFlags.cmake:225
/wd5105was removed, but this file enables/WXfor MSVC debug builds (CMAKE_CXX_FLAGS_DEBUG), and Windows SDK headers are known to emit C5105 regardless of the conforming preprocessor flag. This can turn into a hard build break for users. Keep/wd5105alongside/Zc:preprocessor(or gate its removal on a known-safe Windows SDK/toolset version).
# Enable the conforming preprocessor to support C++20 features.
#
# See also:
# * https://devblogs.microsoft.com/cppblog/announcing-full-support-for-a-c-c-conformant-preprocessor-in-msvc/
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Zc:preprocessor")
|
@kou Please take a look. |
|
Ah, let's upgrade bundled Boost before this: #51116 |
There was a problem hiding this comment.
🔵 Needs a closer look
Removing /wd5105 and unconditionally adding /Zc:preprocessor can cause MSVC/Windows SDK builds (especially Debug with /WX) to fail without a compatibility guard.
Review details
Suppressed comments (1)
cpp/cmake_modules/SetupCxxFlags.cmake:225
- Dropping
/wd5105is likely to reintroduce warning C5105 from Windows SDK headers; in Debug builds this file enables/WX, so that warning can become a hard build failure. Also,/Zc:preprocessorshould be added only if the current compiler (including clang-cl) supports it, consistent with other flag probes in this file.
# Enable the conforming preprocessor to support C++20 features.
#
# See also:
# * https://devblogs.microsoft.com/cppblog/announcing-full-support-for-a-c-c-conformant-preprocessor-in-msvc/
set(CXX_COMMON_FLAGS "${CXX_COMMON_FLAGS} /Zc:preprocessor")
- Files reviewed: 1/1 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@github-actions crossbow submit conda |
|
Revision: c8c6ab1 Submitted crossbow builds: ursacomputing/crossbow @ actions-84903b7129
|
|
Hmm. Let's add support for LLVM 23.1 before this to check this with conda: #51245 |
Rationale for this change
Try to use MSVC's new preprocessor to handle C++20 grammar such as
__VA_OPT__.What changes are included in this PR?
Change the common flag under MSVC.
Are these changes tested?
Tested by existed tests.
Are there any user-facing changes?
No.
__VA_OPT__#50978