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

Add some macros from html5ever #5

Merged
merged 3 commits into from Feb 27, 2015
Merged

Add some macros from html5ever #5

merged 3 commits into from Feb 27, 2015

Conversation

kmcallister
Copy link
Collaborator

These are licensed MIT/ASL2.

These are adapted [1] from html5ever, which is licensed MIT/ASL2.

I wasn't sure if you planned for people to use this library as

    #[macro_use]
    extern crate mac;

i.e. with no list of specific macros to import. For that reason I gave these
macros an "ext_" prefix, as they are unlikely to be useful outside of writing
syntax extensions.

[1]: https://github.com/servo/html5ever/blob/master/macros/src/internal.rs
@reem
Copy link
Owner

reem commented Feb 27, 2015

I relicensed the crate under MIT/Apache-2.0 under a suggestion from reddit.

Reviewing this now.

/// # }
/// ```
#[macro_export]
macro_rules! format_if {
Copy link
Owner

Choose a reason for hiding this comment

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

I look at this macro and I see a verbosity switch. The really cool thing here is being able to switch being verbose (full, formatted, allocated error) and quick (short, borrowed error).

In light of this, it might be neat to re-use the verbosity levels and setup from the log crate and branch on that. If the logging setup isn't ideal, we can also just use the verbosity levels and leave them unconnected. Thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's an interesting idea. The log crate levels don't really match the html5ever use case, though. You don't need this if you're printing thru the log crate, only when reporting errors up to a library client.

We can merge the simple version now and think about this more.

Copy link
Owner

Choose a reason for hiding this comment

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

Agreed. I'll make an issue.

@reem
Copy link
Owner

reem commented Feb 27, 2015

Left some thoughts on format_if, everything else looks great.

@reem
Copy link
Owner

reem commented Feb 27, 2015

Actually, I'm curious why the syntax-extension-related macros need to be macros at all, they look like they could be functions.

@kmcallister
Copy link
Collaborator Author

ext_bail does an early return from the invoking function, and the others invoke ext_bail.

@reem
Copy link
Owner

reem commented Feb 27, 2015

Oh, my mistake, I missed the return in ext_bail.

#[macro_export]
macro_rules! ext_bail_if {
($e:expr, $cx:expr, $sp:expr, $msg:expr) => {{
if $e { bail!($cx, $sp, $msg) }
Copy link
Owner

Choose a reason for hiding this comment

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

should be ext_bail

@reem
Copy link
Owner

reem commented Feb 27, 2015

Can you add tests for these macros, which can be as simple as just calling them once? Right now there's almost no checking for if any of these macros will actually compile when expanded (see typo of bail instead of ext_bail.)

@kmcallister
Copy link
Collaborator Author

The doctests are live, but the syntax ext macros don't have any.

@reem
Copy link
Owner

reem commented Feb 27, 2015

Doctests are good then - I won't make you make doctests for the syntax ext macros because that sounds painful.

@kmcallister
Copy link
Collaborator Author

I plan to start using these in h5e as soon as the PR is merged, so we'll find out if they break :)

reem added a commit that referenced this pull request Feb 27, 2015
Add some macros from html5ever
@reem reem merged commit 63084a5 into reem:master Feb 27, 2015
@reem
Copy link
Owner

reem commented Feb 27, 2015

Woo!

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.

None yet

2 participants