-
Notifications
You must be signed in to change notification settings - Fork 324
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
Automatically force all-defaulted functions #3414
Conversation
| foo f = f 1 | ||
| bar x=1 = x + 1 | ||
| foo (bar ...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick but I'd suggest making the argument passed to f
have different value than the default one - this way we ensure that we do not get some weird behaviour where the default one was used instead (it seems extremely unlikely to get a bug with such behaviour, but you never know and the cost of testing this case is 0 as we just need to change this value).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I wouldn't call it "extremely unlikely" given the behavior I saw in the process of implementing this, and I would call the test as it exists "extremely bad at testing things" :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it runs fast, then it is probably good enough. As I don't yet understand the code base much, I provided some general comments and few questions. I see no problem integrating the request.
limit = Constants.CacheSizes.THUNK_EXECUTOR_NODE) | ||
Stateful doCached( | ||
Thunk thunk, | ||
Function thunk, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit pick: does the variable still deserve name thunk
when it's type is now Function
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that thunk.isThunk
is a bit confusing :)
|
||
@Specialization( | ||
guards = {"!fn.isThunk()", "fn.isFullyApplied()", "isTail == cachedIsTail"}, | ||
limit = "3" /* limit is 3 because that's the number of different values for isTail */) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One could define
static int numberOfDifferentValuesForIsTail() {
return BaseNode.TailStatus.values().length;
}
and then use
limit = "3" /* limit is 3 because that's the number of different values for isTail */) | |
limit = "numberOfDifferentValuesForIsTail()" |
engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/argument/Thunk.java
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/FunctionSchema.java
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/callable/function/FunctionSchema.java
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/java/org/enso/interpreter/runtime/type/Types.java
Show resolved
Hide resolved
...me/src/main/java/org/enso/interpreter/node/callable/argument/IndirectArgumentSorterNode.java
Show resolved
Hide resolved
val dependencies = depInfo.dependencies | ||
|
||
// The test for dependents | ||
dependents.getDirect(argXId) shouldEqual Some(Set(xUseId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: Why the changes in this test have to be made? Is it because of changes in DemandAnalysis
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's a horrible terrible spec and honestly I had to stop myself nuking the whole file. So the problem here is, these stupid pass specs use previous passes to build the initial test state. So, demand analysis runs before this, so the input IR for this spec changes and now it blows up. We should discuss this, but IMO in the near future we need to either nuke all of these specs, or rewrite them to be self-sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the IR
serializable? If so, we can serialize the input into a file and read it.
engine/runtime/src/test/scala/org/enso/interpreter/test/semantic/CurryingTest.scala
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
Outdated
Show resolved
Hide resolved
engine/runtime/src/main/scala/org/enso/compiler/codegen/IrToTruffle.scala
Outdated
Show resolved
Hide resolved
limit = Constants.CacheSizes.THUNK_EXECUTOR_NODE) | ||
Stateful doCached( | ||
Thunk thunk, | ||
Function thunk, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that thunk.isThunk
is a bit confusing :)
…uffle.scala Co-authored-by: Hubert Plociniczak <hubert.plociniczak@gmail.com>
Reviewed again. Looks sane. |
Pull Request Description
This changes the interpreter to treat functions with all-defaulted args as thunks. Seems to have no performance impact in compiled code.
Important Notes
Checklist
Please include the following checklist in your PR:
./run dist
and./run watch
.