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 error explanation for E0010 #24892

Merged
merged 1 commit into from
Apr 30, 2015
Merged

Conversation

robinst
Copy link
Contributor

@robinst robinst commented Apr 28, 2015

Part of #24407.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pcwalton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

@@ -168,6 +168,11 @@ match x {
```
"##,

E0010: r##"
The value of static and const variables must be known at compile time, and they
live for the entire lifetime of a program. They can not use allocation.
Copy link
Member

Choose a reason for hiding this comment

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

The word "allocation" already occurs in the message currently emitted for E0010. Rather than "They can not use allocation" (which will not enlighten someone who did not already know what the first message meant), I would say "They are not allowed to create Box values", maybe?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about "They are not allowed to allocate values using box"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a longer comment about allocation on the heap being a run-time operation would be ideal. Mentioning Box values could also be good but the core concept is allocation.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if that second sentence were this?

Creating a boxed value allocates memory on the heap at run-time, and therefore cannot be done at compile-time.

Copy link
Member

Choose a reason for hiding this comment

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

that looks good to me, especially I think this error code is currently only emitted for uses of box (and not other methods of dynamic memory allocation, all of which would currently also be illegal to even attempt to encode).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1, done.

@alexcrichton
Copy link
Member

r? @pnkfelix

@rust-highfive rust-highfive assigned pnkfelix and unassigned pcwalton Apr 28, 2015
@bors
Copy link
Contributor

bors commented Apr 29, 2015

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

@michaelsproul
Copy link
Contributor

@robinst: If you add that second sentence and do a rebase we can merge this 😄

@@ -168,6 +168,11 @@ match x {
```
"##,

E0010: r##"
The value of static and const variables must be known at compile time, and they
Copy link
Contributor

Choose a reason for hiding this comment

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

‘const variable’ is an oxymoron. This should probably just read something like ‘The value of constants (declared with either const or static) must be known…’

Copy link
Member

Choose a reason for hiding this comment

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

I think using 'constants' is worse, since statics are not constants (either in theory or in practice).

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps ‘The value of constants and statics must be known…’?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or maybe?

Both constants and statics are given values at compile-time. In the case of constants, the value is fixed for the duration of the program's execution, while statics are given an initial value in the program binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done using @P1start's suggestion.

@pnkfelix
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Apr 30, 2015

📌 Commit 95ad630 has been approved by pnkfelix

@bors
Copy link
Contributor

bors commented Apr 30, 2015

⌛ Testing commit 95ad630 with merge f9ecc6e...

bors added a commit that referenced this pull request Apr 30, 2015
@bors bors merged commit 95ad630 into rust-lang:master Apr 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants