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 a test to verify that TrustedPromise does not implement Clone #14500

Closed
jdm opened this issue Dec 8, 2016 · 6 comments
Closed

Add a test to verify that TrustedPromise does not implement Clone #14500

jdm opened this issue Dec 8, 2016 · 6 comments

Comments

@jdm
Copy link
Member

@jdm jdm commented Dec 8, 2016

It is not a safe operation to clone a TrustedPromise, but it's really easy to derive Clone on the type and make it possible. We should add a test in https://dxr.mozilla.org/servo/source/tests/compiletest/plugin/compile-fail like:

fn cloneable<T: Clone>() {
}

fn main() {
    cloneable::<TrustedPromise>();
}
@jdm jdm added the E-easy label Jan 11, 2017
@highfive
Copy link

@highfive highfive commented Jan 11, 2017

Please make a comment here if you intend to work on this issue. Thank you!

@Verlet64
Copy link
Contributor

@Verlet64 Verlet64 commented Jan 14, 2017

I'm a little green with Rust, but this seems a good place to start. I can take it on.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 14, 2017

Great! Please ask questions if anything is unclear, and be sure to verify that the test works by deriving a Clone implementation for TrustedPromise.

@jdm jdm added the C-assigned label Jan 14, 2017
@Verlet64
Copy link
Contributor

@Verlet64 Verlet64 commented Jan 22, 2017

@jdm I did it, but I would like some advice on the naming of the file before I submit any pull request etc. At the moment, it's the very verbose trustedpromise_mustnot_derive_clone.rs and though it conveys exactly what the test is for...it's very verbose, and if I'm honest doesn't seem to quite fit the naming convention the project has gone for.

Help from more experienced hands would be much appreciated here.

Thanks.

@jdm
Copy link
Member Author

@jdm jdm commented Jan 22, 2017

Perhaps not_cloneable.rs?

@Verlet64
Copy link
Contributor

@Verlet64 Verlet64 commented Jan 22, 2017

That sounds fine.

I'll clean up then and go through the pull request checklist.

bors-servo added a commit that referenced this issue Jan 22, 2017
Added compiletest to verify TrustedPromise does not implement Clone

As per issue: #14500

I have added a test to ensure that TrustedPromise does not implement Clone.

This is my first PR to the project, so feedback in terms of both code and the actual PR would be very welcome.

Thanks,
Verlet64

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14500
- [x] There are tests for these changes

<!-- 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/15146)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 4, 2017
… not implement Clone (from Verlet64:verlet64/no_clone_trusted_promise); r=jdm

As per issue: servo/servo#14500

I have added a test to ensure that TrustedPromise does not implement Clone.

This is my first PR to the project, so feedback in terms of both code and the actual PR would be very welcome.

Thanks,
Verlet64

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14500
- [x] There are tests for these changes

Source-Repo: https://github.com/servo/servo
Source-Revision: 27a7999d5d85c9b1156554f597cb0eea592624e1
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
… not implement Clone (from Verlet64:verlet64/no_clone_trusted_promise); r=jdm

As per issue: servo/servo#14500

I have added a test to ensure that TrustedPromise does not implement Clone.

This is my first PR to the project, so feedback in terms of both code and the actual PR would be very welcome.

Thanks,
Verlet64

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14500
- [x] There are tests for these changes

Source-Repo: https://github.com/servo/servo
Source-Revision: 27a7999d5d85c9b1156554f597cb0eea592624e1

UltraBlame original commit: fad6c9d357894eee05cd6cd11c904590ddc86e9f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
… not implement Clone (from Verlet64:verlet64/no_clone_trusted_promise); r=jdm

As per issue: servo/servo#14500

I have added a test to ensure that TrustedPromise does not implement Clone.

This is my first PR to the project, so feedback in terms of both code and the actual PR would be very welcome.

Thanks,
Verlet64

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14500
- [x] There are tests for these changes

Source-Repo: https://github.com/servo/servo
Source-Revision: 27a7999d5d85c9b1156554f597cb0eea592624e1

UltraBlame original commit: fad6c9d357894eee05cd6cd11c904590ddc86e9f
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
… not implement Clone (from Verlet64:verlet64/no_clone_trusted_promise); r=jdm

As per issue: servo/servo#14500

I have added a test to ensure that TrustedPromise does not implement Clone.

This is my first PR to the project, so feedback in terms of both code and the actual PR would be very welcome.

Thanks,
Verlet64

---
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #14500
- [x] There are tests for these changes

Source-Repo: https://github.com/servo/servo
Source-Revision: 27a7999d5d85c9b1156554f597cb0eea592624e1

UltraBlame original commit: fad6c9d357894eee05cd6cd11c904590ddc86e9f
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.

3 participants
You can’t perform that action at this time.