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

replace usages of snprintf, sscanf #179

Merged
merged 23 commits into from
Nov 19, 2019

Conversation

Mrmaxmeier
Copy link
Collaborator

@Mrmaxmeier Mrmaxmeier commented Oct 30, 2019

This closes #192

dpx/src/dpx_pdfobj.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@cormacrelf cormacrelf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GitHub is forcing me to leave a comment

@Mrmaxmeier Mrmaxmeier force-pushed the cleanup-shims branch 2 times, most recently from 71b826a to 83a02e5 Compare November 16, 2019 21:31
@cormacrelf
Copy link
Collaborator

Maybe it’s a good idea to add some tests for the version parser? It looks better but it’s hard to tell from just looking. Would help if you wanted to change it to nom later as well.

@burrbull
Copy link
Collaborator

I like these changes, but not sure about ptr::null(). It hides variable type and can complicate future refactoring.

Same also with PdfObjRef in #176 .

@Mrmaxmeier
Copy link
Collaborator Author

This now includes the commits from #191 that got lost in #192.
I'll drop a6e7530 and 697712f if we decide to prefer 0 as *const _ over ptr::null()

@burrbull
Copy link
Collaborator

If it is already in master, I'm not against.

@crlf0710
Copy link
Owner

Personally i think ptr::null() is fine at this stage ... and they'll disappear when we convert them into references.

@cormacrelf
Copy link
Collaborator

Doesn’t every c2rust-generated “0 as *const _” have a corresponding explicit type annotation anyway, even in method bodies? I have been keeping the annotation but replacing the 0 with ptr::null.

@crlf0710
Copy link
Owner

Seems good. Just need to fix compile errors on Mac (seems extern types doesn't work well together with ptr::null()?) and Windows (are we still calling vsprintf from somewhere?), and we're good to go~

fastmod -m -d . --extensions rs \
  "pdf_add_stream\(\s*([^\n]*?),\s+?b(\".*?)\\\\x00\".*?,\s+.*?\)"
  "pdf_add_stream_str(\${1}, \${2}\")"
This allows for exotic PDF with this header:

00000000: 2550 4446 2d31 2e34 204d 6172 7665 6c6c  %PDF-1.4 Marvell
00000010: 2053 656d 6963 6f6e 6475 6374 6f72 0a31   Semiconductor.1
With checked (i32) adds, this panicked during format generation.
@Mrmaxmeier Mrmaxmeier merged commit 978276a into crlf0710:oxidize Nov 19, 2019
@Mrmaxmeier Mrmaxmeier deleted the cleanup-shims branch November 19, 2019 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A few changes rolled back
4 participants