Fix and reject some warning-classes when building#9708
Fix and reject some warning-classes when building#9708rawler wants to merge 4 commits intobambulab:masterfrom
Conversation
- Remove unnecessary std::move() calls that prevent RVO/NRVO (1978 warnings) - Fix misleading indentation warnings - Fix catch-by-value exception handling in AppConfig.cpp and STEP.cpp - Fix tab/space indentation inconsistencies Reduces compiler warnings from 5618 to 3359 (40% reduction).
…h-value Enforce compile-time errors for three classes of bugs: - -Werror=pessimizing-move: Prevent std::move() that blocks RVO/NRVO - -Werror=misleading-indentation: Catch code that looks guarded but isn't - -Werror=catch-value: Prevent exception slicing via catch-by-value
- Fix return-reference-to-temporary bugs in GUI_App.cpp and Config.cpp - Fix mismatched new[]/delete memory corruption in WebViewDialog.cpp - Fix self-move bug in Model.cpp - Fix dangling-else ambiguity in Model.cpp, GCodeProcessor.cpp, ClipperUtils.cpp, Brim.cpp - Fix parentheses precedence in Point.cpp, GCodeProcessor.cpp, BambuStudio.cpp - Fix function address comparison bug in BambuStudio.cpp - Fix useless null check on reference in AxisCtrlButton.cpp - Add default cases to switch statements in MeshBoolean.cpp
…elete, dangling-else Extend compile-time error checking for additional bug classes: - -Werror=return-local-addr: Returning reference/pointer to local variable - -Werror=self-move: Moving object to itself - -Werror=mismatched-new-delete: new[]/delete or new/delete[] mismatch - -Werror=dangling-else: Ambiguous else clause
|
Full Disclosure; These commits were created with the help of Claude, but I have gone over them all myself, trying to avoid contributing to low-quality AI slop PR:s. |
| // skip the filaments change in machine start/end gcode | ||
| if (machine_start_gcode_end_line_id == (unsigned int)(-1) && (unsigned int)(line_id)<machine_start_gcode_end_line_id || | ||
| machine_end_gcode_start_line_id != (unsigned int)(-1) && (unsigned int)(line_id)>machine_end_gcode_start_line_id) | ||
| if ((machine_start_gcode_end_line_id != (unsigned int)(-1) && (unsigned int)(line_id) < machine_start_gcode_end_line_id) || |
There was a problem hiding this comment.
This made me terribly uncertain. The way I read the first line is that any time machine_start_gcode_end_line_id would be INT_MAX, we would abort? But that surely can't be intended?
There was a problem hiding this comment.
i looked at this about a month ago and was confused as well ;)
There was a problem hiding this comment.
The intention is to skip all filament-change G-code included inside the machine’s start and end G-code sections.
The variables start_gcode_end_line and end_gcode_start_line denote the line indices where the start G-code ends and the end G-code begins, respectively, and are initialized to -1.
A line should be skipped if:
- start_gcode_end_line is still -1 (we haven’t passed the start G-code yet), or
- end_gcode_start_line has been set (not -1) and the current line_id is greater than end_gcode_start_line (we are inside the end G-code).
| return 0; | ||
| static const double s_zero = 0.0; | ||
| return s_zero; |
There was a problem hiding this comment.
This is an example of an actual bug being fixed. Returning 0 here, AFAIU means returning a reference to a stack-allocated variable containing 0, which is illegal and can break if caller calls something else before dereferencing this functions return value. (It triggered return-local-addr warning)
lanewei120
requested review from
Diatom-Banbu,
bambu123 and
zhimin-zeng-bambulab
3 participants
Building Bambu Studio generates thousands of warnings. A few of these represents real bugs, I.E. using "null" for a reference is illegal C++, and causes undefined behavior. Others are just noise, and contributing to a bad state of broken windows.
Trying to improve the situation in this codebase, the first two commits of this PR fixes a few of the "simple & safe" classes of warnings, such as
pessimizing-move,misleading-indentationandcatch-value, and turns these warnings into errors - to avoid future reappearance.The second two commits continues with warnings that require slightly more complex analysis, but should still be fairly safe to fix without introducing new problems. Here are a few cases where the warnings represent actual bugs (illegal C++), such as returning a reference to a stack-local null.