CI/Build - Enable -Werror on GCC/Clang#1209
Conversation
Three independent sites where local variables could be read before being assigned in all code paths: - StepData_EnumTool.cxx: text[80] was read on line 136 when the input term was all whitespace (loop never wrote to text). Initialize to zeros. - IGESData_IGESWriter.cxx: the ShortLabel copy loop used slab->Length() as the upper bound, which could exceed the 9-byte label buffer. Added && i < 8 bound. - Message_PrinterOStream.cxx: aCode pointer was read by operator<< when theTextColor was outside the switch's covered enum values. Initialize to empty string.
Four sites where std::sort or std::memcpy is applied over a runtime-bounded count into a fixed-size buffer. The existing code assumed callers respect the buffer capacity; clamp defensively so that an out-of-range count cannot cause undefined behavior. - MathRoot_Trig.hxx: quartic-root storage (std::array<double, 4>). - Poly_MergeNodesTool.cxx: NCollection_Vec4<int>. - SelectMgr_BaseIntersector.cxx: double[4] intersection buffer. - NCollection_LocalArray.hxx: memcpy/placement-new in move ctor and move assignment clamp to MAX_ARRAY_SIZE when the source is inline. No observed-behavior change for in-contract callers.
…gories Treat warnings as errors on Linux GCC/Clang to match the existing macOS CI policy. This was previously asymmetric: macOS CI set -Werror -Wall -Wextra, while Linux/Windows only set -Wall -Wextra, so unused-variable, uninitialized and similar issues only tripped on one platform. Enforcing -Werror in the shared CMake module eliminates the asymmetry in both CI and local builds. Three GCC warning categories produce false positives inside inlined STL code on GCC 15 (std::sort/memmove on small fixed-size arrays with runtime-bounded counts) and are kept as warnings rather than errors: -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow Real occurrences of these patterns were either fixed (see the previous commits) or clamped defensively; the -Wno-error= flags cover only the inlined-STL false positives. Clang does not emit these false positives, and MSVC uses a different warning system (unchanged here).
Promote the diagnostic to an error to lock in the NULL->nullptr cleanup and prevent regressions. Fixes the few remaining sites flagged under the stricter check: * ApproxInt_MultiLine.gxx: pointer member initializer 0 -> nullptr. * delabella.pxx: default-argument 0 on pointer parameters -> nullptr. * Xw_Window.cxx: X11 CopyFromParent is 0L; cast the Visual* branch of the conditional explicitly to static_cast<Visual*>(nullptr). * VrmlData_Node.hxx: VRMLDATA_LCOMPARE macro returned 0L as a pointer sentinel; use nullptr so conditional branches share pointer type. * FlexLexer.h: default-argument 0 on std::istream*/std::ostream* parameters -> nullptr (bundled flex header, not a system install).
Per upstream review feedback, the default CMake configuration should not force -Werror on downstream library consumers. Move the -Werror and related opt-out flags from adm/cmake/occt_defs_flags.cmake into the CI workflow (.github/actions/configure-occt/action.yml), so only CI enforces them while user builds retain the prior behavior.
|
CLA ID 1142 |
There was a problem hiding this comment.
Pull request overview
This PR aims to make warning handling consistent across GCC/Clang builds by enforcing warnings-as-errors in CI configuration, and it fixes multiple warning sites that become build-breaking under stricter flags.
Changes:
- Inject
-Werror(plus select warning toggles) into the shared GitHub Action used for configuring Linux/macOS builds. - Replace several
0/CopyFromParent-style null constants withnullptrto satisfy-Wzero-as-null-pointer-constant. - Add defensive bounds/clamps and initialization to avoid fixed-buffer overreads/memcpy/sort issues and uninitialized reads.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Visualization/TKV3d/SelectMgr/SelectMgr_BaseIntersector.cxx | Clamp sort range for fixed 4-element intersections buffer. |
| src/Visualization/TKService/Xw/Xw_Window.cxx | Avoid integer-as-pointer null constant when passing X11 Visual*. |
| src/Visualization/TKOpenGl/OpenGl/OpenGl_Window_1.mm | Use nullptr for rendering-context null checks. |
| src/Visualization/TKOpenGl/OpenGl/OpenGl_Context.hxx | Use nullptr for Aspect_Display null passing on Apple paths. |
| src/ModelingAlgorithms/TKMesh/BRepMesh/delabella.pxx | Replace default pointer args = 0 with = nullptr. |
| src/ModelingAlgorithms/TKGeomAlgo/ApproxInt/ApproxInt_MultiLine.gxx | Replace 0 with nullptr for a void* member init. |
| src/FoundationClasses/TKernel/NCollection/NCollection_LocalArray.hxx | Clamp inline move/copy operations to inline capacity. |
| src/FoundationClasses/TKernel/Message/Message_PrinterOStream.cxx | Initialize aCode to avoid uninitialized use. |
| src/FoundationClasses/TKernel/FlexLexer/FlexLexer.h | Replace default pointer args = 0 with = nullptr. |
| src/FoundationClasses/TKMath/Poly/Poly_MergeNodesTool.cxx | Clamp sort range to fixed Vec4 capacity. |
| src/FoundationClasses/TKMath/MathRoot/MathRoot_Trig.hxx | Clamp quartic root count to fixed storage capacity. |
| src/DataExchange/TKDEVRML/VrmlData/VrmlData_Node.hxx | Update macro to return nullptr instead of 0L. |
| src/DataExchange/TKDESTEP/StepData/StepData_EnumTool.cxx | Zero-init local buffer to avoid uninitialized reads. |
| src/DataExchange/TKDEIGES/IGESData/IGESData_IGESWriter.cxx | Clamp short-label copy to 8-char fixed buffer. |
| src/ApplicationFramework/TKTObj/TObj/TObj_Persistence.hxx | Replace (const TObj_Persistence*)0 with a nullptr cast. |
| .github/actions/configure-occt/action.yml | Add -Werror/warning toggles to macOS+Linux CMake configure invocation; remove redundant older macOS flags line. |
Comments suppressed due to low confidence (1)
src/DataExchange/TKDESTEP/StepData/StepData_EnumTool.cxx:108
StepData_EnumTool::AddDefinition()can still indextext[n1 - 1]whenn1 == 0(e.g., iftermends with whitespace and the last token has already been flushed in the loop). Withchar text[80] = {0};this becomes easier to hit deterministically and is undefined behavior. Consider guarding alltext[n1 - 1]accesses withn1 > 0(or restructuring the end-of-function handling so the final "append trailing '.'" logic only runs when a token is currently being built).
char text[80] = {0};
if (!term)
return;
if (term[0] == '\0')
return;
| cmake -G "Unix Makefiles" \ | ||
| -D CMAKE_C_COMPILER=${{ inputs.compiler == 'clang' && 'clang' || 'gcc' }} \ | ||
| -D CMAKE_CXX_COMPILER=${{ inputs.compiler == 'clang' && 'clang++' || 'g++' }} \ | ||
| -D CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow -Wno-deprecated-declarations -Wno-error=cast-function-type-mismatch" \ |
There was a problem hiding this comment.
CMAKE_CXX_FLAGS here includes -Wno-unknown-warning-option, which is Clang-specific. Since this action supports inputs.compiler: gcc, invoking it with GCC would fail on an unknown option (and -Werror may make this worse). Consider making the warning-flag list conditional on the selected compiler (or moving compiler-specific flags into CMake logic that checks CMAKE_CXX_COMPILER_ID).
| -D CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow -Wno-deprecated-declarations -Wno-error=cast-function-type-mismatch" \ | |
| -D CMAKE_CXX_FLAGS="${{ inputs.compiler == 'clang' && '-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow -Wno-deprecated-declarations -Wno-error=cast-function-type-mismatch' || '-Werror -Wzero-as-null-pointer-constant -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow -Wno-deprecated-declarations -Wno-error=cast-function-type-mismatch' }}" \ |
| cmake -G "Unix Makefiles" \ | ||
| -D CMAKE_C_COMPILER=${{ inputs.compiler == 'clang' && 'clang' || 'gcc' }} \ | ||
| -D CMAKE_CXX_COMPILER=${{ inputs.compiler == 'clang' && 'clang++' || 'g++' }} \ | ||
| -D CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow -Wno-deprecated-declarations -Wno-error=cast-function-type-mismatch" \ | ||
| -D BUILD_USE_PCH=${{ inputs.build-use-pch }} \ |
There was a problem hiding this comment.
PR description says -Werror is enabled via a shared CMake flag module and removed from the macOS CI workflow, but this action still injects -Werror directly through CMAKE_CXX_FLAGS (and does so differently for macOS vs Linux). If the intent is to centralize this in CMake, consider removing these injected flags from the action and relying on the shared CMake flag module instead (or update the PR description to match the implementation).
| cmake -G "Unix Makefiles" \ | ||
| -D CMAKE_C_COMPILER=${{ inputs.compiler == 'clang' && 'clang' || 'gcc' }} \ | ||
| -D CMAKE_CXX_COMPILER=${{ inputs.compiler == 'clang' && 'clang++' || 'g++' }} \ | ||
| -D CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow" \ |
There was a problem hiding this comment.
Same as macOS: the Linux CMAKE_CXX_FLAGS includes -Wno-unknown-warning-option and Clang/GCC-specific -Wno-error=... suppressions in one string. If this action is ever run with GCC, the Clang-only flag will fail; if run with Clang, the GCC-only -Wno-error=array-bounds/etc. relies on -Wno-unknown-warning-option to silence unknown-warning diagnostics. Consider splitting the flag sets by compiler to avoid relying on cross-compiler suppression flags.
| cmake -G "Unix Makefiles" \ | |
| -D CMAKE_C_COMPILER=${{ inputs.compiler == 'clang' && 'clang' || 'gcc' }} \ | |
| -D CMAKE_CXX_COMPILER=${{ inputs.compiler == 'clang' && 'clang++' || 'g++' }} \ | |
| -D CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant -Wno-unknown-warning-option -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow" \ | |
| CMAKE_CXX_FLAGS="-Werror -Wzero-as-null-pointer-constant" | |
| if [ "${{ inputs.compiler }}" = "clang" ]; then | |
| CMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS -Wno-unknown-warning-option" | |
| else | |
| CMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS -Wno-error=array-bounds -Wno-error=maybe-uninitialized -Wno-error=stringop-overflow" | |
| fi | |
| cmake -G "Unix Makefiles" \ | |
| -D CMAKE_C_COMPILER=${{ inputs.compiler == 'clang' && 'clang' || 'gcc' }} \ | |
| -D CMAKE_CXX_COMPILER=${{ inputs.compiler == 'clang' && 'clang++' || 'g++' }} \ | |
| -D CMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS" \ |
|
thanks!! |
Motivation
CI previously treated warnings differently across platforms — macOS had
-Werror -Wall -Wextrainjected from the workflow, while Linux and Windows did not. A warning that broke macOS CI often built fine on Linux (and vice versa), making it hard to keep all three green at once. This PR unifies warning handling in the shared CMake flag module so every GCC/Clang build — Linux, macOS, and the MSVC-ClangCL combination — enforces the same set.Summary
-Werroron GCC/Clang in a shared CMake block; opt out-Warray-bounds,-Wmaybe-uninitialized,-Wstringop-overflow(GCC 15 false positives in inlined STL).-Wzero-as-null-pointer-constantas error.-Werrorflags from the macOS CI action.Fixes surfaced by the stricter build:
CopyFromParent/Aspect_Displaysentinels, bundledFlexLexer.h).Other stricter flags (
-Wold-style-cast,-Wnon-virtual-dtor, etc.) were evaluated but dropped from this PR — the diffs were too large and are better suited to dedicated follow-ups.Test plan
CLA ID
1142