Skip to content

[Do not merge, partial change to discuss] Have gecko_string_cache use &Atom instead of BorrowedAtom#12548

Closed
SimonSapin wants to merge 1 commit intomasterfrom
gecko-string-cache
Closed

[Do not merge, partial change to discuss] Have gecko_string_cache use &Atom instead of BorrowedAtom#12548
SimonSapin wants to merge 1 commit intomasterfrom
gecko-string-cache

Conversation

@SimonSapin
Copy link
Copy Markdown
Member

@SimonSapin SimonSapin commented Jul 22, 2016 •
edited by larsbergstrom
Loading

This is part of a larger change discussed at #12391 (comment), but I’d like to get feedback on this specifically first.

This change is Reviewable

@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 22, 2016

The idea is to replace BorrowedAtom<'a> with &'a WeakAtom, given pub struct WeakAtom(nsIAtom);.

WeakAtom is not Copy or Clone and only ever used behind a &WeakAtom reference so that you can’t get it on the stack, and that reference itself is the *mut nsIAtom pointer.

Atom dereferences to WeakAtom, which is more in line with how String and &str work.

Using a &T reference is important to make things generic in the selectors crate since it uses a HashMap with atom keys. HashMap supports lookups with a key of a different type from the one stored, though the std::borrow::Borrow trait:

pub trait Borrow<Borrowed: ?Sized> {
    fn borrow<'a>(&'a self) -> &'a Borrowed;
}

Note that &'a Borrowed is returned, not Borrowed<'a> since higher-kinded type constructors are only possible in current rust through advanced traits trickery.

SimonSapin
SimonSapin reviewed Jul 22, 2016
View reviewed changes
ports/geckolib/string_cache/lib.rs
pub struct Atom(*mut nsIAtom);
#[derive(PartialEq, Eq, Debug, Hash, Clone)]
pub struct Namespace(pub Atom);
pub type Namespace = Atom;
Copy link
Copy Markdown
Member Author

@SimonSapin SimonSapin Jul 22, 2016

Choose a reason for hiding this comment

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

This was mostly to get things building more easily. I can re-introduce the newtype and add WeakNamespace.

@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 22, 2016

The part that I think is kinda fishy is struct WeakAtom(nsIAtom); and using &self as a raw pointer.

f? @bholley @emilio @heycam

@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 22, 2016 •
edited
Loading

Full change set, if you want more context:

@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 22, 2016

@bholley To respond to your IRC question: nsIAtom is not unsized, and the reason not to use it directly is that its fields are public and we don’t want users to mess with them:

ports/geckolib/gecko_bindings/structs_*.rs

#[repr(C)]
#[derive(Debug, Copy)]
pub struct nsIAtom {
    pub _base: nsISupports,
    pub _bitfield_1: u32,
    pub mHash: u32,
    /**
   * WARNING! There is an invisible constraint on |mString|: the chars it
   * points to must belong to an nsStringBuffer. This is so that the
   * nsStringBuffer::FromData() calls above are valid.
   */
    pub mString: *mut ::std::os::raw::c_ushort,
}

@bholley
Copy link
Copy Markdown
Contributor

bholley commented Jul 22, 2016

I would prefer to add some bindgen magic to restrict those fields - see rust-lang/rust-bindgen#20 (comment).

@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 22, 2016

We can always change to that once bindgen supportis there.

@bholley
Copy link
Copy Markdown
Contributor

bholley commented Jul 22, 2016

@Manishearth is adding that support right now, so maybe we can just use nsIAtom and add the belt-and-suspenders privacy annotations shortly afterwards? Would be less overall churn.

@SimonSapin SimonSapin mentioned this pull request Jul 23, 2016
Merged
5 tasks
@SimonSapin
Copy link
Copy Markdown
Member Author

SimonSapin commented Jul 23, 2016

Changes in dependencies being merged and published, I’ve opened #12571. Let’s move this discussion there.

@SimonSapin SimonSapin closed this Jul 23, 2016
@SimonSapin SimonSapin deleted the gecko-string-cache branch July 23, 2016 20:22
bors-servo pushed a commit that referenced this pull request Aug 5, 2016
Auto merge of #12571 - servo:selectors-generic-atom_, r=bholley
6bc7136 
Update to selectors 0.8 (generic over atoms)

This removes the `[replace]` override in geckolib and therefore unblocks #12391.

This includes the `gecko_string_cache` redesign discussed in #12548.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12571)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Aug 9, 2016
Auto merge of #12571 - servo:selectors-generic-atom_, r=bholley
ffb0631 
Update to selectors 0.8.1 (generic over atoms)

This removes the `[replace]` override in geckolib and therefore unblocks #12391.

This includes the `gecko_string_cache` redesign discussed in #12548.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12571)
<!-- Reviewable:end -->
bors-servo pushed a commit that referenced this pull request Aug 9, 2016
Auto merge of #12571 - servo:selectors-generic-atom_, r=bholley
5e83b3f 
Update to selectors 0.8.2 (generic over atoms)

This removes the `[replace]` override in geckolib and therefore unblocks #12391.

This includes the `gecko_string_cache` redesign discussed in #12548.

r? @bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/12571)
<!-- Reviewable:end -->
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #12571 - Update to selectors 0.8.2 (generic over atoms) …
e9c3e6f 
…(from servo:selectors-generic-atom_); r=bholley

This removes the `[replace]` override in geckolib and therefore unblocks servo/servo#12391.

This includes the `gecko_string_cache` redesign discussed in servo/servo#12548.

r? bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 5e83b3f83bfcf48d0096442bdf5c9bf753623970

UltraBlame original commit: db13dfd0cfa9ee8de452d6083c2d5bdda74585af
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #12571 - Update to selectors 0.8.2 (generic over atoms) …
dce3336 
…(from servo:selectors-generic-atom_); r=bholley

This removes the `[replace]` override in geckolib and therefore unblocks servo/servo#12391.

This includes the `gecko_string_cache` redesign discussed in servo/servo#12548.

r? bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 5e83b3f83bfcf48d0096442bdf5c9bf753623970

UltraBlame original commit: db13dfd0cfa9ee8de452d6083c2d5bdda74585af
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this pull request Oct 1, 2019
@marco-c
servo: Merge #12571 - Update to selectors 0.8.2 (generic over atoms) …
5e43447 
…(from servo:selectors-generic-atom_); r=bholley

This removes the `[replace]` override in geckolib and therefore unblocks servo/servo#12391.

This includes the `gecko_string_cache` redesign discussed in servo/servo#12548.

r? bholley

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #12548 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require new tests because refactor

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 5e83b3f83bfcf48d0096442bdf5c9bf753623970

UltraBlame original commit: db13dfd0cfa9ee8de452d6083c2d5bdda74585af
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SimonSapin @bholley