Conversation
Three warning sites in the library, plus the leftovers from the review.
-Wreorder linear_elasticity.h: m_eps_name was declared after m_K/m_G
but initialized before them, and m_eps is wired FROM it.
Members initialize in declaration order regardless of how the
initializer list reads, so this worked by luck of m_eps being
last. Declarations now match the list.
-Wshadow material_base.h: two constructor parameters named after the
type aliases they shadow.
unused json_material_factory.h: a typedef nothing read.
[[nodiscard]] on local_newton::solve and vector_newton::converged, which
backward_euler already had. A dropped convergence flag is the bug this
codebase has had most often.
material_base::m_material_handler was stored and never read -- confirmed
across include, tests and examples. The constructor parameter stays,
because object_store passes it.
tmech, nlohmann and Eigen become SYSTEM includes. Their warnings were
never ours to act on and they were burying the three that were: tmech
alone emits dozens of -Wshadow. This is what makes -Werror possible at
all -- without it the CI gate below would have failed on a dependency we
do not control, on its first run.
With the gate on, it immediately found nine more in our own TESTS: local
aliases duplicating the file-scope one, an unused handle, and an unbraced
if around EXPECT_NEAR (which expands to an if/else, so it was a real
dangling else). The Dim-parameterized `tensor2` in test_j2_plasticity is
NOT redundant -- the file-scope one is fixed at 3 -- so it is renamed
rather than removed.
One of those fixes nearly introduced a bug, which is the argument for the
gate. test_statev_map's isotropic4(T K, T G) shadowed the file-scope K and
G; renaming the parameters left the body reading K and G, which silently
rebound to the file-scope constants, so the function would have ignored
its arguments. The only caller passes exactly those values, so no test
outcome would have changed -- luck, not correctness.
CI now builds with -Wall -Wextra -Wshadow -Wreorder and -Werror for the
three cleaned classes. Not a blanket -Werror: that would fail on any new
warning class the compiler adds.
300/300 with -Werror active.
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.
Closes #76.
The library warnings
-Wreorderlinear_elasticity.h:m_eps_namewas declared afterm_K/m_Gbut initialized before them, andm_epsis wired from it. Members initialize in declaration order regardless of how the initializer list reads, so this worked only by luck ofm_epsbeing last.-Wshadowx2material_base.h: two constructor parameters named after the type aliases they shadow.json_material_factory.hPlus
[[nodiscard]]onlocal_newton::solveandvector_newton::converged(backward_euleralready had it) -- a dropped convergence flag is the bug this codebase has had most often -- and the deadm_material_handler, confirmed unread acrossinclude,testsandexamples. The constructor parameter stays, sinceobject_storepasses it.Why SYSTEM includes were needed first
I added
-Werrorfor the cleaned classes, then found every remaining warning came from tmech, dozens of-Wshadowamong them. As written, the CI change would have failed on a dependency we do not control, on its first run.tmech, nlohmann and Eigen are now
SYSTEMincludes. Their warnings were never ours to act on, and they were burying the three that were.The gate found nine more, in our tests
Local aliases duplicating the file-scope one, an unused handle, and an unbraced
ifaroundEXPECT_NEAR-- which expands to anif/else, so it was a genuine danglingelse.The
tensor2intest_j2_plasticityis not redundant: the file-scope one is fixed at 3 while that one is parameterized onDim. Renamed rather than removed.One fix nearly introduced a bug
This is the argument for the gate.
test_statev_map.cpphad:Renaming the parameters to
bulk/shearleft the body readingKandG, which silently rebound to the file-scope constants166.67/76.92-- the function would have ignored its arguments entirely. Caught on inspection and repointed.The only caller passes exactly those values, so no test outcome would have changed either way. That is luck, not correctness, and it is precisely what
-Wshadowis for.CI
-Wall -Wextra -Wshadow -Wreorder, with-Werrorfor the three cleaned classes only. Deliberately not a blanket-Werror, which would fail on any new warning class a future compiler adds.300/300 with
-Werroractive.Note on overlap
This touches
CMakeLists.txt(thetarget_include_directorieslines) and #81 touches it too (the install section). They should merge cleanly; I will verify the merge result locally before either lands rather than assume it.