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.

Sign up for GitHub

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

Jump to bottom

apfloat: improve doc comments #63416

Merged
merged 1 commit into from
Oct 2, 2019
Merged

apfloat: improve doc comments #63416

merged 1 commit into from
Oct 2, 2019

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 9, 2019

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 9, 2019
@eddyb
Copy link
Member

eddyb commented Aug 10, 2019 •
edited
Loading

I'd rather not change rustc_apfloat directly, but rather upstream it to LLVM and then port it back to rustc_apfloat. I need to go through the few changes that were made to rustc_apfloat and not reviewed by me, to make sure there aren't more instances like this.

@RalfJung
Copy link
Member Author

Submitting patches to LLVM is super painful though. :/
So what do we do with this PR?

@RalfJung
Copy link
Member Author

@eddyb so what do we do with this PR now? Just close it? Seems like a waste of a good docs improvement, but then it's not like his took long to write.

@JohnCSimon
Copy link
Member

hello from triage,
@RalfJung it seems that you'll just have to close this PR, sorry.
Thank you.

@RalfJung
Copy link
Member Author

I'd like for @eddyb to lay down a plan for how we can improve apfloat when that is needed. AFAIK patches have landed here before without being sync'ed upstream, and AFAIK there is no precedent for making code "read-only" like that in rustc.

Cc @rust-lang/compiler

@eddyb
Copy link
Member

eddyb commented Aug 24, 2019

I'm really not sure how to proceed, there are annoying constraints at odds.
Maybe if we moved this to a separate repo (#55993) we could have:

  • one branch for the port (where LLVM->Rust backports would go into)
  • one branch for the crates.io version, based on the other branch, with some other patches on top

@RalfJung
Copy link
Member Author

Given that we have landed doc fixes for apfloat before, I don't understand why we are stopping now.

@JohnCSimon
Copy link
Member

Ping from triage

@RalfJung @eddyb This has sat idle for a week ... I'm not sure what to do with this one.

Thanks

@RalfJung
Copy link
Member Author

RalfJung commented Sep 9, 2019

Neither am I, and @rust-lang/compiler hasn't chimed in either...

@Mark-Simulacrum Mark-Simulacrum added I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2019
@pnkfelix
Copy link
Member

@RalfJung that may be in part because this PR does not have a team label.

@pnkfelix pnkfelix added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 26, 2019
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Sep 26, 2019 •
edited
Loading

My two cents:

The changes seem good, but @eddyb seemed to feel that it would help @skade in working out licensing issues if we make the most minimal changes we can to these files. I think that's a perfectly good reason to hold off on this PR. (And prior precedent was basically in error.)

(If, on the other hand, the licensing issues don't get easier/harder based on whether we've made changes, then... I say land them. =)

@eddyb eddyb added S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. and removed I-nominated S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Sep 26, 2019
@skade
Copy link
Contributor

skade commented Sep 30, 2019

I think it's okay to land this, if @RalfJung agrees on future relicensing of his effort.

@RalfJung
Copy link
Member Author

RalfJung commented Oct 1, 2019

Sure, I am happy to relicense this particular PR under any license.

@eddyb eddyb removed the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Oct 1, 2019
@eddyb eddyb mentioned this pull request Oct 1, 2019
Closed
@eddyb
Copy link
Member

eddyb commented Oct 1, 2019

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Oct 1, 2019

📌 Commit 5faae38 has been approved by eddyb

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Oct 1, 2019
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
@Centril
Rollup merge of rust-lang#63416 - RalfJung:apfloat, r=eddyb
185f8a9 
apfloat: improve doc comments

r? @eddyb @nagisa
@Centril Centril mentioned this pull request Oct 1, 2019
Closed
bors added a commit that referenced this pull request Oct 1, 2019
@bors
Auto merge of #64958 - Centril:rollup-8k5m97o, r=Centril
8d0ebb2 
Rollup of 5 pull requests

Successful merges:

 - #63416 (apfloat: improve doc comments)
 - #64404 (Add long error explanation for E0495)
 - #64910 (syntax: cleanup param, method, and misc parsing)
 - #64912 (Remove unneeded `fn main` blocks from docs)
 - #64952 (Update cargo.)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
@tmandry
Rollup merge of rust-lang#63416 - RalfJung:apfloat, r=eddyb
afc2bcc 
apfloat: improve doc comments

r? @eddyb @nagisa
@tmandry tmandry mentioned this pull request Oct 1, 2019
Closed
tmandry added a commit to tmandry/rust that referenced this pull request Oct 1, 2019
@tmandry
Rollup merge of rust-lang#63416 - RalfJung:apfloat, r=eddyb
431c075 
apfloat: improve doc comments

r? @eddyb @nagisa
@tmandry tmandry mentioned this pull request Oct 1, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
@Centril
Rollup merge of rust-lang#63416 - RalfJung:apfloat, r=eddyb
e15de04 
apfloat: improve doc comments

r? @eddyb @nagisa
@Centril Centril mentioned this pull request Oct 1, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Oct 1, 2019
@Centril
Rollup merge of rust-lang#63416 - RalfJung:apfloat, r=eddyb
baba628 
apfloat: improve doc comments

r? @eddyb @nagisa
@Centril Centril mentioned this pull request Oct 1, 2019
Merged
bors added a commit that referenced this pull request Oct 1, 2019
@bors
Auto merge of #64972 - Centril:rollup-gcawast, r=Centril
7130fc5 
Rollup of 7 pull requests

Successful merges:

 - #63416 (apfloat: improve doc comments)
 - #64820 (BTreeSet intersection, is_subset & difference optimizations)
 - #64910 (syntax: cleanup param, method, and misc parsing)
 - #64912 (Remove unneeded `fn main` blocks from docs)
 - #64933 (Fixes #64919. Suggest fix based on operator precendence.)
 - #64943 (Add lower bound doctests for `saturating_{add,sub}` signed ints)
 - #64950 (Simplify interners)

Failed merges:

r? @ghost
@bors bors merged commit 5faae38 into rust-lang:master Oct 2, 2019
@RalfJung RalfJung deleted the apfloat branch October 9, 2019 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eddyb eddyb eddyb approved these changes

Assignees

@eddyb eddyb

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@RalfJung @eddyb @JohnCSimon @pnkfelix @nikomatsakis @skade @bors @Mark-Simulacrum @rust-highfive