- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 764
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add and use const_ptr_api macro in openssl-sys #1359
Conversation
It seems I actually got it working. Any opinions regarding the syntax? I think putting the cfg flags on the specific If you really want the cfg flags as attribute on the fn item, it probably would have to be the first attribute (otherwise it will take more recursions just to find it). Also I think ctest will ignore any macro expansions failures (I had |
} | ||
const_ptr_api! { | ||
extern "C" { | ||
pub fn ASN1_STRING_to_UTF8(out: *mut *mut c_uchar, s: #[const_ptr_if(any(ossl110, libressl280))] ASN1_STRING) -> c_int; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can the macro be changed to only apply to the individual extern definition rather than wrapping the whole extern "C" block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntactically speaking I think the macro can do that, but rustc doesn't like it; playground example fails with error[E0428]: the name
ASN1_STRING_to_UTF8 is defined multiple times
; but when looking at the generated code through Tools
-> Expand macros
it seems to be doing the right thing.
I'm pretty sure that is the reason why the cfg_if!
blocks have internal extern "C" {...}
blocks too; probably the same limitation. And even if rustc
got fixed and you raise the MSRV, ctest will still fail, as it uses an old fork if a rustc internal crates to do parsing and macro expansion.
Passing extern "C" { ... }
can be removed of course, but it can't be part of another extern "C" { ... }
block and needs to emit extern "C" { ... }
for the fn items.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah bummer. This seems good then!
Nice! Yeah, the cfg syntax in your macro seems fine. Just to make sure I understand - ctest does still include tests for the macro-defined functions, right? It just doesn't fail to compile if the macro is misused but the normal openssl-sys build would fail at that point anyway? |
After I removed the As ctest uses an (unmaintained) old fork of a rustc internal crate the current rustc won't necessarily fail (also it seems to use a hardcoded recursion limit of 128). |
3bec303
to
2c42cef
Compare
so it also compiles on rust 1.31; also same limit as used by systest/ctest.
2c42cef
to
1dbeb8c
Compare
macos seems to be down in circleci... I also opened gnzlbg/ctest#101 - if this gets merged and released, I think |
Alternative approach to #1348; actually tries to parse fn declarations and magically inserts
*const
or*mut
.