Conversation
There was a problem hiding this comment.
Pull request overview
This pull request titled "Skh/safer string ops" contains a diverse set of changes that go beyond string operations, including removal of legacy macOS-specific code, fixes to debug output operators, thread safety improvements, and CI runner updates.
Changes:
- Removed legacy macOS/TARGET_OS_MAC preprocessor conditionals from third-party libraries (zlib, libpng, freetype)
- Fixed compilation errors in embree debug output operators by removing references to non-existent struct members
- Added thread safety macros to ProjectSettings methods (get_resource_path, localize_path, globalize_path)
- Removed deprecated AGL framework from macOS build configuration
- Updated GitHub Actions workflow to use macos-15-intel runners
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| thirdparty/zlib/zutil.h | Removed obsolete MACOS/TARGET_OS_MAC preprocessor block |
| thirdparty/libpng/pngpriv.h | Removed Mac-specific math.h include conditional |
| thirdparty/freetype/src/gzip/ftzconf.h | Removed MACOS conditional around Byte typedef |
| thirdparty/embree/kernels/subdiv/bezier_curve.h | Fixed QuadraticBezierCurve output operator by removing non-existent member references |
| thirdparty/embree/kernels/geometry/pointi.h | Fixed PointMi output operator by removing non-existent member reference |
| platform/osx/detect.py | Removed deprecated AGL framework from link flags |
| core/project_settings.cpp | Added THREAD_SAFE_METHOD macros to three path-related methods |
| .github/workflows/fnx.yaml | Updated macOS runner from macos-13 to macos-15-intel |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| friend embree_ostream operator<<(embree_ostream cout, const QuadraticBezierCurve& a) { | ||
| return cout << "QuadraticBezierCurve ( (" << a.u.lower << ", " << a.u.upper << "), " << a.v0 << ", " << a.v1 << ", " << a.v2 << ")"; | ||
| return cout << "QuadraticBezierCurve ( (), " << a.v0 << ", " << a.v1 << ", " << a.v2 << ")"; |
There was a problem hiding this comment.
The output operator for QuadraticBezierCurve now prints empty parentheses "()" instead of the original interval data. The QuadraticBezierCurve struct only has members v0, v1, and v2 (defined at line 111), and does not have a member 'u' with 'lower' and 'upper' fields. The original code attempting to print "a.u.lower" and "a.u.upper" would have caused a compilation error. However, replacing this with empty parentheses "()" makes the debug output less useful. Consider either removing the empty parentheses entirely or replacing them with meaningful data that actually exists in the struct, such as the interval computed by the interval() method.
| return cout << "QuadraticBezierCurve ( (), " << a.v0 << ", " << a.v1 << ", " << a.v2 << ")"; | |
| return cout << "QuadraticBezierCurve ( " << a.interval() << ", " << a.v0 << ", " << a.v1 << ", " << a.v2 << ")"; |
| friend __forceinline embree_ostream operator<<(embree_ostream cout, const PointMi& line) | ||
| { | ||
| return cout << "Line" << M << "i {" << line.v0 << ", " << line.geomID() << ", " << line.primID() << "}"; | ||
| return cout << "Line" << M << "i {" << ", " << line.geomID() << ", " << line.primID() << "}"; |
There was a problem hiding this comment.
The output operator for PointMi now omits printing the first field. Looking at the PointMi struct definition (lines 224-229), there is no member named 'v0'. The original code trying to print "line.v0" would have caused a compilation error. However, the fix leaves two consecutive commas in the output string, which produces malformed output like "LineMi { , geomID, primID}". Consider either removing the extra comma and space, or replacing the missing field with an actual member that should be printed for debugging purposes.
| return cout << "Line" << M << "i {" << ", " << line.geomID() << ", " << line.primID() << "}"; | |
| return cout << "Line" << M << "i {" << line.geomID() << ", " << line.primID() << "}"; |
No description provided.