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

Initial implementation of const fn #25609

Merged
merged 5 commits into from
May 24, 2015
Merged

Conversation

nikomatsakis
Copy link
Contributor

This is a port of @eddyb's const-fn branch. I rebased it, tweaked a few things, and added tests as well as a feature gate. The set of tests is still pretty rudimentary, I'd appreciate suggestions on new tests to write. Also, a double-check that the feature-gate covers all necessary cases.

One question: currently, the feature-gate allows the use of const functions from stable code, just not the definition. This seems to fit our usual strategy, and implies that we might (perhaps) allow some constant functions in libstd someday, even before stabilizing const-fn, if we were willing to commit to the existence of const fns but found some details of their impl unsatisfactory.

r? @pnkfelix

@nikomatsakis
Copy link
Contributor Author

Oh, I've run tests locally, but not since rebasing. make check is running now.

@@ -8,6 +8,10 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// TODO list:
Copy link
Contributor

Choose a reason for hiding this comment

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

tidy fails on this, it should be FIXME

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it should not be a FIXME. I always put TODOs intentionally so that I actually am forced to DO the things in question. In this case, though, I DID do that, but forgot I made the TODO list, so I should just remove the comment...

// except according to those terms.

// Test that we can't declare a const fn in an impl -- right now it's
// just not allowed at all, though eventualy it'd make sense to allow
Copy link
Member

Choose a reason for hiding this comment

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

s/eventualy/eventually

@bors
Copy link
Contributor

bors commented May 19, 2015

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

@nikomatsakis
Copy link
Contributor Author

Rebased. But when I run make check locally I get an overflow error (investigating...)

eddyb and others added 4 commits May 21, 2015 11:47
- add feature gate
- add basic tests
- adjust parser to eliminate conflict between `const fn` and associated
constants
- allow `const fn` in traits/trait-impls, but forbid later in type check
- correct some merge conflicts
@nikomatsakis
Copy link
Contributor Author

Passes everything now.

abi: self.fty.abi
abi: self.fty.abi,

// trait methods canot (currently, at least) be const
Copy link
Member

Choose a reason for hiding this comment

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

typo: cannot

@pnkfelix
Copy link
Member

@nikomatsakis if you're still looking for suggestions of tests to add, I noticed that while you have both a + b and f(args) as const expressions, I did not see any tests of a method call of the form a.f(b) in a const expression.

You might also add tests that uses of conditional (if, if let, match) and looping forms (for,while,loop) are rejected inconst fn; I didn't see that either. (Maybe I overlooked it -- I did see thatlet` itself seems to be rejected...)

@pnkfelix
Copy link
Member

It overall looks good to me; I'm going to r+ it and we can add more tests later, just to avoid future rebase pain.

@pnkfelix
Copy link
Member

@bors r+ 82ded3c

@bors
Copy link
Contributor

bors commented May 24, 2015

⌛ Testing commit 82ded3c with merge ba0e1cd...

bors added a commit that referenced this pull request May 24, 2015
This is a port of @eddyb's `const-fn` branch. I rebased it, tweaked a few things, and added tests as well as a feature gate. The set of tests is still pretty rudimentary, I'd appreciate suggestions on new tests to write. Also, a double-check that the feature-gate covers all necessary cases.

One question: currently, the feature-gate allows the *use* of const functions from stable code, just not the definition. This seems to fit our usual strategy, and implies that we might (perhaps) allow some constant functions in libstd someday, even before stabilizing const-fn, if we were willing to commit to the existence of const fns but found some details of their impl unsatisfactory.

r? @pnkfelix
@Ericson2314
Copy link
Contributor

From what I remember from the RFC, there was less consensus that const functions were the right way long term than that they were the best stop-gap for the time being. That would seemingly point to making them only usable as normal functions from stable, so unstable users can get all the benefits of them without forcing a decision on what we want to do long-term. Finally, if we want to commit to the existence of const functions, we can easily just allow them to be used again from stable code, and std will be ready to go.

@nikomatsakis nikomatsakis deleted the const-fn branch March 30, 2016 16:12
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.

7 participants