The move to macros is great! As a coding style issue, are we encouraging the style of ns!(xml) or ns!("xml")?

Reviewed 17 of 17 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

build.rs, line 70 [r1] (raw file):
Add std::slice and std::mem to the imports list?

src/atom/mod.rs, line 308 [r1] (raw file):
I don't see how this fits in a u64: one byte for length + 7 bytes data + 1 bit for the tag = 65 bits. Add a unit test for this?

src/lib.rs, line 56 [r1] (raw file):
Are we encouraging ns!("xml") or ns!(xml)?

Comments from the review on Reviewable.io

Review status: 15 of 16 files reviewed at latest revision, 3 unresolved discussions.

build.rs, line 70 [r1] (raw file):
Done.

src/atom/mod.rs, line 308 [r1] (raw file):
The length is stored in the upper 4 bits of the same bytes that has the tag, which is enough since it only goes up to 7: https://github.com/servo/string-cache/blob/0854229fbe/src/atom/mod.rs#L341

Are the tests at https://github.com/servo/string-cache/blob/0854229fbe/src/atom/mod.rs#L465-L466 enough?

src/lib.rs, line 56 [r1] (raw file):
The former is not supported anymore. But this choice was fairly arbitrary. If you prefer it I can change the macro.

Comments from the review on Reviewable.io

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

src/atom/mod.rs, line 308 [r1] (raw file):
Oh silly me, the unpacked representation is 65 bits, it then packs down to 64.

Since you're being explicit about packing to u64 I don't think you need a unit test.

src/lib.rs, line 56 [r1] (raw file):
Perhaps a conversation on irc to see if anyone has opinions? I slightly prefer ns!("xml") but I don't have a strong opinion.

Comments from the review on Reviewable.io

☔ The latest upstream changes (presumably #126) made this pull request unmergeable. Please resolve the merge conflicts.

Here is how it works in this last commit and why:

• atom!(…) only accepts quoted strings. That string’s value is exactly the value that the atom will get (through Deref<Target=str>), so it makes sense to use the syntax of &str string literals.
• ns!(…) only accepts unquoted identifiers. The identifier is a namespace prefix like svg that is typically used but it is not the value that ends up in the atom, which is the namespace URL like "http://www.w3.org/2000/svg". Identifiers are used to name values.

This removes alternative/synonymous syntax based on "there should be one way of doing it". (This may cause more churn for updating html5ever and Servo, though.)

It's an identifier, not a string, so I agree with not quoting the argument to ns!().

Reviewed 5 of 5 files at r3.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

Comments from the review on Reviewable.io

@bors-servo r+

🔑 Insufficient privileges

@bors-servo r=asajeffrey

📌 Commit 4d505d6 has been approved by asajeffrey

⌛ Testing commit 4d505d6 with merge 76d6428...

☀️ Test successful - travis