Remove obsolete warning disables from base_macros.h - #1625
Remove obsolete warning disables from base_macros.h#1625Daniel Jump (DanielJump) wants to merge 3 commits into
Conversation
base_macros.h disables C5046, C4268, C4499 and C4630 without a matching
#pragma warning(push)/(pop) pair. base.h includes it near the top and never
restores the warning state, so those four warnings stay disabled for the rest
of every translation unit that includes base.h, not just for the C++/WinRT
declarations they were added for.
Consumers silently lose the warnings in their own code:
namespace { struct S { int x; }; }
S f();
int main() { f(); }
That warns C5046 on its own, and stops warning as soon as <winrt/base.h> is
included ahead of it, at both /W4 and /Wall.
Open the scope before the include and close it at the end of base.h. The
disables still cover everything C++/WinRT declares, and the warning state is
handed back to the consumer unchanged. The set of warnings reported from
within the C++/WinRT headers is unaffected.
There was a problem hiding this comment.
🟢 Approval recommended
The change safely scopes MSVC warning disables to the generated header and is low risk, with only a minor comment-accuracy nit noted.
Pull request overview
This PR fixes warning-state leakage from <winrt/base.h> by scoping the MSVC warning disables originating in base_macros.h so they apply to C++/WinRT’s own declarations but don’t remain disabled for downstream consumer code.
Changes:
- Add a generator-emitted
#pragma warning(push)immediately beforebase_macros.his included in the generatedbase.h. - Ensure a matching
#pragma warning(pop)is emitted at the end of the generatedbase.hvia the existingfinish_withRAII writer pattern. - Document the intended pairing behavior in
strings/base_macros.h.
File summaries
| File | Description |
|---|---|
| strings/base_macros.h | Adds commentary explaining intended warning-disable scoping behavior for MSVC. |
| cppwinrt/file_writers.h | Wraps the base_macros root-include in a push/pop warning scope when generating winrt/base.h. |
| cppwinrt/code_writers.h | Introduces a reusable writer RAII helper that emits #pragma warning(push) and guarantees a matching pop. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
If these apply only to base.h, then why not move them to base.h? |
|
I doubt whether C++/WinRT still needs to keep these pragmas, as support for VS2017 and VS2019 may have long since ceased to exist. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The C5046 and C4268 disables targeted Visual C++ 15.9 and 16.3, while current v143 and v145 builds no longer emit them. The v145 module build also no longer emits C4499 or C4630. Delete the pragmas instead of adding push/pop plumbing so the warning-state leak is removed at its source. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: a5731c31-939d-42bb-bf1d-45099cd5dd53
Daniel Jump (DanielJump)
changed the title
|
Charles Milette (@sylveon) Yexuan Xiao (@YexuanXiao) You are both right to question preserving these pragmas. I tested removing them instead. The v143 regular header build no longer emits C5046 or C4268, and the v145 named-module build no longer emits C4499 or C4630. Both build with |
|
Correction to my previous validation note: I have now checked the official MSVC off-by-default list and rebuilt under |
|
I suggest changing it to: #if defined(_MSC_VER) && defined(WINRT_IMPL_BUILD_MODULE)
// C++ module warnings by /W4
#pragma warning(disable : 4499)
#pragma warning(disable : 4630)
#endif // _MSC_VERbecause all module files define WINRT_IMPL_BUILD_MODULE before including base_macro.h and header mode does not need them. C++ modules do not leak macros, so they do not need to be pushed/popped. As for the other two warnings, if they no longer occur in VS2022, then perhaps it's better to simply remove them, since C++/WinRT is no longer tested against VS2017 and VS2019. |
|
I tested base.h and base_macro.h using https://godbolt.org/z/jajMTveT5, and VS17.0 no longer produces the erroneous 4499 and 4630 warnings with /W4 and /WX enabled. |
|
Thanks for checking VS 17.0 as well. That matches the v143 and v145 results here: neither C4499 nor C4630 is emitted for the module path anymore. Since the supported header and module toolsets are clean for all four warnings, I will leave the PR as the simpler deletion rather than retain module-only suppressions. |
Fixes #1624.
base_macros.hglobally disables C5046, C4268, C4499 and C4630, so every translation unit that includes<winrt/base.h>also loses those diagnostics in its own code.The first two pragmas are documented in the source as workarounds for Visual C++ 15.9 and 16.3. The latter two were added for early C++ module support. Current supported toolsets no longer emit any of the four warnings for these sources, so this removes the pragmas instead of adding push/pop plumbing around them.
The resulting change is only 12 deleted lines.
Validation
/Wallwith both v143 and v145 without any of the four warnings./Wallwithout any of the four warnings./W4 /WXbuilds and tests remain clean.<winrt/base.h>.MSVC warning-level reference: https://learn.microsoft.com/cpp/build/reference/compiler-option-warning-level