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

Use a macro to create null-terminated C strings #19916

Closed
jdm opened this issue Jan 31, 2018 · 8 comments
Closed

Use a macro to create null-terminated C strings #19916

jdm opened this issue Jan 31, 2018 · 8 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Jan 31, 2018

When we write them by hand (eg. b"some string\0"), we invariably get them wrong in ways that are tricky to notice (#19915). We should use a macro like this instead:

macro_rules! c_str {
    ($str:expr) => {
        concat!($str, "\0").as_bytes()
    }
}

This would allow us to write code like (c_str!("PEParseDeclarationDeclExpected"), Action::Skip) instead of https://github.com/emilio/servo/blob/d82c54bd3033cc3277ebeb4854739bebe4e20f2f/ports/geckolib/error_reporter.rs#L237. We should be able to clean up all of the uses in that file.

No need to run any automated tests; if it builds with ./mach build-geckolib, then it's good enough for a pull request.

@highfive
Copy link

@highfive highfive commented Jan 31, 2018

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

@thiagopnts
Copy link
Contributor

@thiagopnts thiagopnts commented Jan 31, 2018

@highfive: assign me

@highfive
Copy link

@highfive highfive commented Jan 31, 2018

Hey @thiagopnts! Thanks for your interest in working on this issue. It's now assigned to you!

@highfive highfive added the C-assigned label Jan 31, 2018
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 31, 2018

Shouldn't the to_geck_message fn really return a CStr instead in order to at least have some type checking in place?

@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jan 31, 2018

We can also use from_bytes_with_nul for some stronger guarantees, but I don't think the overhead is worth it.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 31, 2018

Returning a CStr does sound like an improvement. The macro could use CStr::from_bytes_with_nul_unchecked under the hood.

@upsuper
Copy link
Member

@upsuper upsuper commented Feb 1, 2018

Hmmm... I didn't notice this issue, and wrote a PR #19925 for fixing this...

@jdm
Copy link
Member Author

@jdm jdm commented Feb 1, 2018

@thiagopnts Given that the solution in #19925 is safer than what I propose here, we're going to go with that. Sorry for the duplication of effort!

@jdm jdm closed this Feb 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.