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

Implement const fn {size,align}_of. #42859

Merged
merged 1 commit into from
Jul 19, 2017

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Jun 23, 2017

Fixes #34078.

r? @nikomatsakis

@petrochenkov petrochenkov added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 23, 2017
@shepmaster shepmaster added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2017
@frewsxcv frewsxcv added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Jun 23, 2017
@eddyb eddyb requested a review from nikomatsakis June 23, 2017 17:46
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

One nit, one question I just wanted to clarify.

#[inline]
#[stable(feature = "rust1", since = "1.0.0")]
#[cfg(not(stage0))]
pub const fn size_of<T>() -> usize {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that I necessarily object to this, but this would be an "insta-stable" change, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change itself yes, but calling any function in a const context still requires #[feature(const_fn)].

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, does it? I thought we enabled calls, but not declarations, of const fns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or at least I couldn't remember which it is.

Copy link
Contributor

Choose a reason for hiding this comment

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

There’s discussion of doing this, but it hasn’t happened yet as far as #24111 says.

Copy link
Contributor

Choose a reason for hiding this comment

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

is_shuffle = true;
}

_ => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add assert!(!self.tcx.is_const_fn(def_id))?

@nikomatsakis
Copy link
Contributor

This is pretty simple. =)

@eddyb
Copy link
Member Author

eddyb commented Jun 27, 2017

@nikomatsakis Yeah, this could've been done months ago...

@nikomatsakis
Copy link
Contributor

on IRC, @eddyb noted that we ought to have some tests. Indeed!

I'm basically happy here. @rust-lang/lang -- everybody happy with making size_of and align_of "const fns"?

@nikomatsakis
Copy link
Contributor

r=me with tests, presuming that it still takes a feature gate to call a const fn

@whitequark
Copy link
Member

Can we unreserve sizeof and alignof now? :D Presumably offsetof actually has to be a keyword, at least I don't see an elegant way to implement it with const fns alone. Could be a builtin macro though.

@eddyb
Copy link
Member Author

eddyb commented Jun 27, 2017

@whitequark I would wait until you can use these functions from constants in stable.
I'm surprised we even bothered to reserve them! I only really want a keyword for typeof 😇.

@whitequark
Copy link
Member

I only really want a keyword for typeof .

What about offsetof? The uses are pretty marginal but when you need them, you do need them...

@eddyb
Copy link
Member Author

eddyb commented Jun 27, 2017

@whitequark I'd just use a macro for that. Or have some sort of static reflection capabilities that allow e.g. "indexing by a field" to access the value, safely etc.

@retep998
Copy link
Member

I really want offsetof because it would make working with unsized structs for FFI so much easier.

@whitequark
Copy link
Member

I really want offsetof because it would make working with unsized structs for FFI so much easier.

I think it shouldn't be a lot of work to implement... I wish I had more time to work on rustc.

@bors
Copy link
Contributor

bors commented Jun 28, 2017

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

@alevy
Copy link
Contributor

alevy commented Jun 28, 2017

This rocks so hard!

@aturon
Copy link
Member

aturon commented Jul 3, 2017

@nikomatsakis sounds great to me!

@durka
Copy link
Contributor

durka commented Jul 5, 2017

[00:10:31] error[E0061]: this function takes 1 parameter but 0 parameters were supplied
[00:10:31]    --> /checkout/src/librustc_const_eval/eval.rs:344:34
[00:10:31]     |
[00:10:31] 344 |           if tcx.type_of(def_id).fn_sig().abi() == Abi::RustIntrinsic {
[00:10:31]     |                                  ^^^^^^ expected 1 parameter
[00:10:31]
[00:10:32] error: aborting due to previous error(s)
[00:10:32]
[00:10:32] error: Could not compile `rustc_const_eval`.

@eddyb eddyb force-pushed the const-size-and-align-of branch 3 times, most recently from 3600acf to 123799d Compare July 5, 2017 10:55
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 6, 2017

📌 Commit 123799d has been approved by nikomatsakis

@eddyb
Copy link
Member Author

eddyb commented Jul 6, 2017

@nikomatsakis There are still no testcases FWIW.

@nikomatsakis
Copy link
Contributor

Still waiting on tests. =)

@eddyb eddyb force-pushed the const-size-and-align-of branch from 1e58b2d to 148718b Compare July 19, 2017 11:47
@eddyb
Copy link
Member Author

eddyb commented Jul 19, 2017

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 19, 2017

📌 Commit 148718b has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Jul 19, 2017

⌛ Testing commit 148718b with merge 42caa5d8031350c501bba1fb32b7ff8b93892474...

@bors
Copy link
Contributor

bors commented Jul 19, 2017

💔 Test failed - status-travis

@retep998
Copy link
Member

Macs are having some issues on travis

The command "curl -fo /usr/local/bin/sccache https://s3.amazonaws.com/rust-lang-ci/rust-ci-mirror/2017-05-12-sccache-x86_64-apple-darwin" failed 3 times.
The command "brew update" failed 3 times.

@kennytm
Copy link
Member

kennytm commented Jul 19, 2017

@retep998 https://www.traviscistatus.com/incidents/74lc35kpxs0y

Due to a regression, some OSX builds have briefly been routed to Linux images. This has change been reverted. We are closely monitoring the situation. Restarting a affected jobs should run the build on the right infrastructure. We apologize for the inconveniences caused.

What's wrong with all the CI problems today… 😕

@Mark-Simulacrum
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jul 19, 2017

⌛ Testing commit 148718b with merge 4ada3a1d0887c79cfbf293fe46d2e7fee18a20ab...

@bors
Copy link
Contributor

bors commented Jul 19, 2017

💔 Test failed - status-appveyor

@retep998
Copy link
Member

I wonder which revocation server is offline and who is responsible for keeping it online?

  [35] SSL connect error (schannel: next InitializeSecurityContext failed: Unknown error (0x80092013) - The revocation function was unable to check revocation because the revocation server was offline.)

@kennytm
Copy link
Member

kennytm commented Jul 19, 2017

@retep998 Probably related https://appveyor.statuspage.io/incidents/m2vdvw39kdk8

The issue is a "broken" route between Rackspace data center and US-based CDN edge server for api.nuget.org. Unfortunately, there is no ETA yet. We continue working with nuget team to resolve the issue as soon as possible.

@alexcrichton
Copy link
Member

alexcrichton commented Jul 19, 2017 via email

@Mark-Simulacrum
Copy link
Member

@bors retry -- appveyor revocation certs, appears to be fixed once #43333 lands.

@bors
Copy link
Contributor

bors commented Jul 19, 2017

⌛ Testing commit 148718b with merge 9bbbd29...

bors added a commit that referenced this pull request Jul 19, 2017
@bors
Copy link
Contributor

bors commented Jul 19, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 9bbbd29 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.