apple / swift-numerics

Advanced mathematical types and functions for Swift
Apache License 2.0
1.67k stars 142 forks source link

Remove conformance of Complex to Differentiable #225

Closed stephentyrone closed 2 years ago

stephentyrone commented 2 years ago

The conformance was breaking CI testing, because the CI bots use a macOS version that predates availability of the _Differentiation module, and the compiler-generated differentation operations have neither availability attributes nor checks, so any reference to those operations simply crashes in dyld because the library is not present. We can add availability, but that's not really accurate either, because these operations are not actually binary-stable, so while we wouldn't crash in dyld, we also might not work correctly in the future. We could mark them unavailable on Apple platforms, but that's also not quite right (and further means that they're not going to get a lot of attention). The best solution seems to be to remove the conformance (which was never source-stable).

stephentyrone commented 2 years ago

@swift-ci test

stephentyrone commented 2 years ago

@swift-ci test

stephentyrone commented 2 years ago

@compnerd This will definitely break the cmake build as is, can you spare a little time to help me fix it?

stephentyrone commented 2 years ago

(In particular, the _NumericsShims target will fail because it no longer has an include subdirectory for the header.)

compnerd commented 2 years ago

Ah, interesting; where is the header supposed to come from now?

stephentyrone commented 2 years ago

It's just _NumericShims/_NumericShims.h now.

compnerd commented 2 years ago

Ah, the CMake change is easy:

diff --git a/Sources/_NumericsShims/CMakeLists.txt b/Sources/_NumericsShims/CMakeLists.txt
index 92ed523..56bef5b 100644
--- a/Sources/_NumericsShims/CMakeLists.txt
+++ b/Sources/_NumericsShims/CMakeLists.txt
@@ -9,6 +9,6 @@ See https://swift.org/LICENSE.txt for license information

 add_library(_NumericsShims INTERFACE)
 target_include_directories(_NumericsShims INTERFACE
-  include)
+  ${CMAKE_CURRENT_SOURCE_DIR})

 set_property(GLOBAL APPEND PROPERTY SWIFT_NUMERICS_EXPORTS _NumericsShims)

This still will break on Windows due to the m linkage. We need to conditionalize that.

stephentyrone commented 2 years ago

This still will break on Windows due to the m linkage. We need to conditionalize that.

Yep. What should it be there (or do we not need an explicit link at all, like on macOS?)

compnerd commented 2 years ago

Correct; just like on macOS, there is no link necessary.

stephentyrone commented 2 years ago

@swift-ci test

compnerd commented 2 years ago

Hmm, I think that the linking approach would work, but missed the CMake change :-(. Nevermind, github gave me a pointer to a mixed up diff.