Export C++20 requirement to CMake consumers - #3971
Conversation
zcbenz
left a comment
There was a problem hiding this comment.
The project requiring C++20 does not mean all the dependents would also need C++20 to use it.
|
Agreed that MLX being built as C++20 does not, by itself, make C++20 a downstream usage requirement. Let's table the JIT/preamble question for now. Setting that aside, the current installed public interface raises a separate, narrow usage-requirement question. At this PR's base, bool operator==(const Device&) const = default;That form of defaulted comparison requires C++20. An include-only probe against the exact base commit: printf '#include "mlx/device.h"\n' |
clang++ -std=c++17 -pedantic-errors -I. -x c++ -fsyntax-only -fails at CMake's compile-feature model uses That appears to leave two coherent downstream contracts:
Which contract is intended? Any follow-up can be scoped specifically to that decision, leaving JIT/preamble out. |
|
That's a valid point thanks for clarification. I agree we should require C++20 since device.h uses the feature, but we should still not require C++20 for JIT compilation which is not yet available in many client machines. |
|
Thanks, understood. The distinction between the installed public C++ interface and the runtime JIT compiler makes sense. I will narrow this PR to the public usage requirement: export cxx_std_20 through the target interface, retain only the consumer-facing CMake/example cleanup, and remove the JIT/preamble changes and associated regression test. I will also update the title, description, and validation to match that scope. |
222a92e to
30a683b
Compare
Advertise cxx_std_20 through the installed target interface because public headers use C++20 defaulted comparisons. Align the shipped external CMake examples and remove stale C++17 target metadata while leaving runtime JIT and preamble compilation on C++17.
30a683b to
98c6a2f
Compare
Proposed changes
MLX's installed public C++ headers use C++20 defaulted comparisons in
DeviceandStream, but the exported CMake target does not advertise thatrequirement.
This exports
cxx_std_20to CMake C++ consumers, aligns the three shippedexternal CMake examples, and removes the stale, non-transitive
CXX_STANDARD 17property from the installed target configuration.Runtime CPU JIT and compiled-preamble compilation remain on C++17.
Validation
-std=c++20,linked, and ran
mlx/device.h:28; the same probepasses under C++20
the extension imported
g++ -std=c++17; native compile coverage passed 31/31cases and 136/136 assertions
pre-commit run --all-filesandgit diff --checkpassedChecklist
pre-commit run --all-filesto format my code / installed pre-commit prior to committing changes