Edits to CHM to remove compiler warnings on Apple Clang 17.0.0 on Intel MacOSX Sequoia 15.7.2 #156
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes compiler warnings on Apple Clang 17.0.0 by addressing unused variable warnings, fixing initialization order issues, and improving the str_format implementation to use standard C++ instead of variable-length arrays.
Key changes:
- Replaced non-standard VLA implementation in
str_format.hwithstd::vectorand added proper error handling - Fixed member initialization order warnings in
ugrid_writerconstructor andcore::output_infoclass - Added virtual destructor to
triangulationbase class for proper polymorphic behavior - Removed unused variables across multiple files to eliminate set-but-not-used warnings
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| third_party/snowpack/snowpackCore/WaterTransport.cc | Removed unused rnN variable that was decremented but never read |
| third_party/snowpack/snowpackCore/ReSolver1d.cc | Removed unused niter_snowpack_dt counter variable |
| third_party/snowpack/Utils.cc | Commented out unused msg_ok variable and nErode counter |
| src/utility/str_format.h | Refactored to use std::vector instead of VLA, added error checking, and updated to more robust implementation |
| src/preprocessing/partition/main.cpp | Removed unused face_end_idx variable from loop |
| src/modules/snow_slide.cpp | Removed unused this_iter_moved_snow flag variable |
| src/mesh/ugrid_writer.cpp | Fixed member initialization order to match declaration order |
| src/mesh/triangulation.hpp | Made destructor virtual for proper polymorphic deletion |
| src/core.hpp | Fixed member initialization order in output_info constructor |
| src/core.cpp | Removed unused chunks counter and trailing newline |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
djmallum
force-pushed
the
fix/compile_warnings_clang
branch
from
ce45880 to
cc1c371
Compare
|
Dealt with the two comments from copilot. Ready to go. |
src/utility/str_format.h
Outdated
|
|
||
|
|
||
|
|
||
| #include <stdexcept> |
There was a problem hiding this comment.
Can you just delete this file and replace it with std::format?
https://stackoverflow.com/a/26221725
the only use of this function is _determine_modeul_dep L2074
There was a problem hiding this comment.
You mean delete the function and file and just use it in the one place it is used?
There was a problem hiding this comment.
I think str_format is only used here
Line 2209 in 92ade51
Can you remove str_format.h entirely and replace with std::format like described here? https://stackoverflow.com/questions/2342162/stdstring-formatting-like-sprintf/26221725#26221725
src/modules/snow_slide.cpp
Outdated
|
|
||
| // only do another iteration if we have incoming mass transport from the ghosts or if we have moved mass this itr | ||
| // this algorithm tends to need a couple passes to make sure there are no straglers | ||
| if(ghost_transport > 0) // || this_iter_moved_snow) |
There was a problem hiding this comment.
Replying to note this in the reivew
|
@Chrismarsh Can you resolve these conflicts? Looks to be related to changes you've made since I made this commit. I handled the conflict in |
|
Sorry about that. Can you check if this still resolves all the warnings? |
Variables set but unused, deleted lines.
`boost::make_shared_object<T>` prints a warning if if `T` has virtual functions it must have a virtual deconstructor.
VLAs are a GCC and Clang extension. Rewrote the function following the linked comment. Removed link, linked to the same stackoverflow question. The new version is simply c++11 complaint. The C++20 version is not supported in all compilers yet.
Lines commented in this commit were triggering a set-but-not-used compiler warning. Previous commits in this branch removed the lines entirely. In this commit this was partially reverted so that lines remain commented with a comment explaining in case of future use.
GDAL CreateField member function to the OGRLayer class (a) does not return void and (b) forces a warning if the output is ignored. The output is now captured and thrown if it fails. Throws on all error types and also outputs the error type. ogr_core.h needs to be checked to see what the error type (enum) actually means.
New warnings were result of some changes that occured before the PR has been merged. Chrismarsh#156
djmallum
force-pushed
the
fix/compile_warnings_clang
branch
from
25366b4 to
15f0f40
Compare
|
I resolved the merge for ugrid witer's ctor, but it might need to be tweaked? |
djmallum
force-pushed
the
fix/compile_warnings_clang
branch
from
512fb76 to
a49af4c
Compare
|
lgtm thanks |
2 participants
I was bored and decided to deal with the warnings that CHM produces during compilation. On my machine, CHM no longer produces compiler warns in the develop branch.
All of these should not impact the code in any way, however, the largest change is in src/utility/str_format.h and should be reviewed/tested to ensure that it encodes the same behaviour.
Finally, I went with removing variables that were set but not used, rather than commenting them out. Can easily revert to just commented out if desired.