Fix PCH visibility settings. - #9455
mcourteaux wants to merge 2 commits into
Conversation
| # The PCH must be compiled with the same visibility settings as the targets | ||
| # that reuse it (via REUSE_FROM, applied in add_halide_test()) -- otherwise | ||
| # Clang rejects the PCH with "default visibility for functions and variables | ||
| # differs in PCH file vs. current file". | ||
| set_target_properties(_test_internal | ||
| PROPERTIES | ||
| CXX_VISIBILITY_PRESET hidden | ||
| VISIBILITY_INLINES_HIDDEN TRUE | ||
| ) |
There was a problem hiding this comment.
We should rather set this:
set(CMAKE_WINDOWS_EXPORT_ALL_SYMBOLS OFF)
set(CMAKE_CXX_VISIBILITY_PRESET hidden)
set(CMAKE_VISIBILITY_INLINES_HIDDEN TRUE)at the top of this file and then remove the analogous block from add_halide_test in HalideTestHelpers.cmake.
This is similar to how the autoscheduler build does it. While we're here, we should change the other instances of CMAKE_VISIBILITY_INLINES_HIDDEN to say TRUE rather than ON or YES for consistency.
There was a problem hiding this comment.
Feel free to commit here what you think is best. I'm not sure I understand fully.
There was a problem hiding this comment.
I'll have Claude interpret this. I'm a bit tired.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9455 +/- ##
==========================================
- Coverage 70.12% 70.01% -0.11%
==========================================
Files 261 261
Lines 79818 79938 +120
Branches 19445 19478 +33
==========================================
Hits 55972 55972
- Misses 17991 18071 +80
- Partials 5855 5895 +40 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Move the visibility settings needed for PCH reuse to directory-scoped variables in test/CMakeLists.txt, matching how the autoscheduler build sets them, and drop the now-redundant per-target visibility settings in add_halide_test(). Also normalize CMAKE_VISIBILITY_INLINES_HIDDEN to TRUE for consistency. Co-Authored-By: Alex Reinking <areinking@adobe.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@alexreinking Claude did it. PTAL. |
2 participants
This was required on my x86 macOS machine with clang.