Skip to content
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

Upgrade hoedown to 3.0.4 #27945

Merged
merged 1 commit into from Aug 25, 2015

Conversation

Projects
None yet
8 participants
@Eljay
Copy link
Contributor

Eljay commented Aug 22, 2015

Some hoedown FFI changes:

  • HOEDOWN_EXT_NO_INTRA_EMPHASIS constant changed.
  • Updated/tidied up all callback function signatures.
  • All opaque data access has an additional layer of indirection for some reason (hoedown_renderer_data).

This also fixes #27862.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Aug 22, 2015

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@@ -68,49 +67,60 @@ const HOEDOWN_EXTENSIONS: libc::c_uint =
type hoedown_document = libc::c_void; // this is opaque to us

This comment has been minimized.

@tamird

tamird Aug 22, 2015

Contributor

while you're here, this should probably be an empty enum

This comment has been minimized.

@daboross

daboross Aug 24, 2015

Contributor

Isn't c_void basically an empty enum? I mean I guess it's represented in hardware by a u8, but what does the difference make?

This comment has been minimized.

@tamird

tamird Aug 24, 2015

Contributor

i believe that c_void is inhabitable; that is, you can create one

This comment has been minimized.

@alexcrichton

alexcrichton Aug 24, 2015

Member

The actual representation or whether this can be constructed isn't so much the problem here, but rather *mut hoedown_document and *mut c_void are the same types. In Rust it's more idiomatic to have those be separate types (to get at least some level of type safety). Creating an empty enum (enum hoedown_document {}) should serve this purpose


#[repr(C)]
struct hoedown_renderer_data {
opaque: *mut libc::c_void,

This comment has been minimized.

@tamird

tamird Aug 22, 2015

Contributor

opaque should probably also be an empty enum, but that might be more work

This comment has been minimized.

@rkruppe

rkruppe Aug 23, 2015

Member

It's a void * in the C code as well, so I think this is fine (or even more appropriate, since pointers to empty enums aren't represented as *i8 in LLVM, while *mut c_void is).

#[repr(C)]
struct hoedown_renderer_data {
opaque: *mut libc::c_void,
}

#[repr(C)]
struct hoedown_renderer {
opaque: *mut libc::c_void,

This comment has been minimized.

@tamird

tamird Aug 22, 2015

Contributor

ditto

@Eljay

This comment has been minimized.

Copy link
Contributor Author

Eljay commented Aug 24, 2015

Changed hoedown_document to be an empty enum and squashed commits.

Adding any kind of type safety to the other void pointers is non-trivial because they point to different types in different passes. A lot of this module is very unsafe/hacky but hopefully we can replace it all with a pure rust Markdown parser at some point anyway!

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Aug 24, 2015

@bors: r+ efc98fa

Thanks!

bors added a commit that referenced this pull request Aug 25, 2015

Auto merge of #27945 - Eljay:upgrade-hoedown, r=alexcrichton
Some hoedown FFI changes:
- `HOEDOWN_EXT_NO_INTRA_EMPHASIS` constant changed.
- Updated/tidied up all callback function signatures.
- All opaque data access has an additional layer of indirection for some reason (`hoedown_renderer_data`).

This also fixes #27862.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 25, 2015

⌛️ Testing commit efc98fa with merge fd7344c...

@bors bors merged commit efc98fa into rust-lang:master Aug 25, 2015

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@Eljay Eljay deleted the Eljay:upgrade-hoedown branch Aug 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.