Fix ExternalTexture_OpenGL throw-stubs to avoid MSVC C4702 under /WX#1703
Open
bkaradzic-microsoft wants to merge 2 commits into
Open
Conversation
…tubs
The OpenGL backend's ExternalTexture::Impl methods (GetInfo, Set, Get)
unconditionally threw std::runtime_error{"not implemented"}, but these
methods are called unconditionally from the dispatch code in
ExternalTexture_Shared.h (included after the class definition).
MSVC's flow analysis inlined the throw and flagged the post-call code
as unreachable, producing C4702 errors under /WX on the
OpenGLWindowsDevOnly build configuration.
A previous change (BabylonJS#1695) masked the warning with a
`target_compile_options(ExternalTexture PRIVATE /wd4702)` gated on
GRAPHICS_API=OpenGL+MSVC. This silenced the diagnostic but hid any
other legitimate C4702 introduced in the same translation unit going
forward.
Attempts to fix this via `[[noreturn]]` on the stub methods made things
worse - MSVC propagated the "never returns" attribute through the
dispatch code, flagging MORE post-call statements as unreachable.
This change instead replaces the throw-stubs with inert no-op stubs
that return default-constructed values. The dispatch code in
ExternalTexture_Shared.h now sees the calls as returning normally,
which eliminates the unreachable-code paths entirely. The /wd4702
workaround in CMakeLists.txt is removed.
Functional impact: Calling an ExternalTexture API on the OpenGL backend
(which is not shipped, only used for the OpenGLWindowsDevOnly developer
build) now yields an inert/null texture rather than a thrown exception.
The OpenGL backend has never supported ExternalTexture, no tests
exercise it on OpenGL, and the Playground does not use it - so this
behavioural change has no test impact.
Verified clean build under Debug + Release + RelWithDebInfo of
ExternalTexture, full Playground build under OpenGL and D3D11
backends. Smoke test passes on D3D11 Release.
This was referenced May 18, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Replaces unimplemented throw stubs in the OpenGL ExternalTexture backend with default-value returns to silence MSVC C4702 (unreachable code) warnings under /WX, and removes the corresponding /wd4702 workaround.
Changes:
- Replace
throw std::runtime_error{"not implemented"}with default returns / empty bodies inExternalTexture_OpenGL.cpp. - Remove the OpenGL+MSVC-specific
/wd4702compile option fromCMakeLists.txt. - Add an explanatory comment describing the rationale for the stub behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp | Convert throwing stubs into inert default-returning stubs and add an explanatory comment. |
| Plugins/ExternalTexture/CMakeLists.txt | Remove the now-unneeded /wd4702 MSVC workaround for the OpenGL build. |
Comment on lines
30
to
40
| return Graphics::TextureT{}; | ||
| } | ||
|
|
||
| private: | ||
| static void GetInfo(Graphics::TextureT, std::optional<Graphics::TextureFormatT>, Info&) | ||
| { | ||
| throw std::runtime_error{"not implemented"}; | ||
| } | ||
|
|
||
| void Set(Graphics::TextureT) | ||
| { | ||
| throw std::runtime_error{"not implemented"}; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
Addressed by 834c99f: the throws are restored, so observable behavior matches the original "not implemented" exception path. The only thing that changes is how MSVC's C4702 is silenced (local pragma in this TU instead of returning early).
Comment on lines
+14
to
+20
| // The OpenGL backend does not implement ExternalTexture. The Impl methods | ||
| // below return default-constructed values rather than throwing so that the | ||
| // shared dispatchers in ExternalTexture_Shared.h (which is included after | ||
| // this class definition) don't produce unreachable-code paths that MSVC | ||
| // flags as C4702 under /WX. Any caller that actually invokes an | ||
| // ExternalTexture API on OpenGL will receive an inert/null texture and | ||
| // get a graphics-level error downstream. |
Contributor
Author
There was a problem hiding this comment.
Addressed by 834c99f: kept the throw and silenced C4702 locally with #pragma warning(push)/disable: 4702/pop around the ExternalTexture_Shared.h include. The rationale (and why [[noreturn]] / inert-return alternatives were rejected) is in the block comment at the top of the file.
…agma
Per code review: working around a compiler warning by changing runtime
behavior is undesirable. Restore the original "not implemented" throws
in the OpenGL backend's ExternalTexture stubs and instead suppress
C4702 only around the #include of ExternalTexture_Shared.h, which is
where the unreachable-code paths are instantiated.
Compared to the previous approach (returning default-constructed values
from the stubs):
- Callers now get a clear "not implemented" diagnostic identical to
every other unimplemented backend path, instead of silently
receiving a null texture.
- The warning suppression is scoped to a few lines of source rather
than the entire translation unit, so future legitimate C4702 in
this file still surfaces under /WX.
Compared to the original TU-wide /wd4702 in CMakeLists.txt (PR BabylonJS#1695):
- Still removes the CMake-level workaround.
- Suppression is now visible at the source site that triggers it,
with comment explaining the rationale and the alternatives that
were rejected ([[noreturn]] propagating through the shared
dispatcher; replacing throws with inert returns changing product
behavior).
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bkaradzic-microsoft
force-pushed
the
weekend/tpc-1652-externaltexture-c4702
branch
from
1d286ee to
834c99f
Compare
ryantrem
reviewed
May 18, 2026
Member
ryantrem
left a comment
ryantrem
left a comment
There was a problem hiding this comment.
The PR description seems out of date with the changes. Is it just moving the warning suppression into the code to make it more restrictive (only part of the file, not the whole file)?
This file contains hidden or 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
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.
Replaces
throw "not implemented"stubs inPlugins/ExternalTexture/Source/ExternalTexture_OpenGL.cppwith inert default-value returns. The dispatcher inExternalTexture_Shared.hno longer trips MSVC C4702 (unreachable code) under/WX, so the/wd4702workaround can be removed fromPlugins/ExternalTexture/CMakeLists.txt.Changes:
Plugins/ExternalTexture/Source/ExternalTexture_OpenGL.cpp- stubs return default-constructed values.Plugins/ExternalTexture/CMakeLists.txt- drop/wd4702.Verified: clean under Debug + Release + RelWithDebInfo on OpenGL and D3D11.
Landing context
This PR is one of 7 splits from the proven CI-green combined preview in draft PR #1702 (see #1702 for the full intended end-state and verified CI run 26044922430).
Recommended landing order
Tier 1 - parallel-reviewable, no source conflicts:
reasonrewrites (5 entries)reasonrewrites (17 entries)Tier 2 - sequential, each touches
Apps/Playground/CMakeLists.txtSCRIPTS list +Apps/Playground/Shared/AppContext.cppLoadScript order; rebase the next branch after the previous merges:Reference policy reminder
Reference PNGs across all 7 PRs come from Babylon.js; never re-baked by BN. Combined diff: 0 PNGs.