Better in headers wasm-c-api #1883
Conversation
syrusakbary
requested review from
Hywan,
jubianchi and
MarkMcCaskey
as code owners
|
bors try |
tryBuild failed: |
lib/c-api/build.rs
Outdated
| // Include the deprecated API for import compatibility reasons | ||
| #include "wasmer_deprecated.h" |
There was a problem hiding this comment.
I don't know what to think about this right now 🤔.
Yup, that's right! |
| @@ -79,6 +79,10 @@ fn build_wasm_c_api_headers(crate_dir: &str, out_dir: &str) { | |||
| #if !defined(WASMER_WASM_H_MACROS) | |||
|
|
|||
| #define WASMER_WASM_H_MACROS | |||
|
|
|||
| // Include the deprecated API for import compatibility reasons | |||
| #include "wasmer_deprecated.h" | |||
There was a problem hiding this comment.
I'd suggest doing this the other way. Make users of the deprecated API #include "wasmer_deprecated.h" which in turn includes (and re-exports) the full #include "wasmer.h". That way the deprecated doesn't need to redeclare any duplicate stuff, and people can easily build to the new API without using old parts by accident, and both APIs are available to users of the old API upgrading parts to the new API.
There was a problem hiding this comment.
We want to do it this way because I don't want to have breaking changes for people that are using the #include "wasmer.h" header for using the API.
That's the main reason for this. We will probably remove this on 2.0 :)
| // Define the `DEPRECATED` macro. | ||
| #if defined(GCC) || defined(__GNUC__) || __has_attribute(deprecated) | ||
| # define DEPRECATED(message) __attribute__((deprecated(message))) | ||
| #elif defined(MSVC) || __has_declspec_attribute(deprecated) | ||
| # define DEPRECATED(message) __declspec(deprecated(message)) | ||
| #endif |
There was a problem hiding this comment.
Yes, perfect! It seems that we should be marking the entire deprecated API with DEPRECATED("use X instead") now?
There was a problem hiding this comment.
It'd be nice to have this macro available for our custom extensions to the Wasmer C API, we have 2 functions related to initializing WASI which are now deprecated and not marked like this, only marked by a comment
|
bors try |
tryBuild failed: |
That's my biggest concern too. With an IDE, a user could just run autocompletion and use functions or types from the deprecated API by mistake :-/. That's really not ideal. And for that, I am likely to stand against this PR if we don't find a correct solution to avoid such scenario. |
|
Closing this PR as it was already merged in master with #2375 |
Description
Previously the
wasmer.hheader corresponded to the (now) deprecated Wasmer API.However, as we move forward and adopted
wasm-c-api, we couldn't usewasmer.hfor it becausewasmer.handwasmer_wasm.hhad conflicting names.Now, this PR solves that by having the old Wasmer API to use the WasmKind enum so people can only use one header always.
Review