Massive changes to replace the site API with a new Invoker based one. #199
Conversation
This change introduced a bug related to non-strict defs. This will be fixed before merge into master. Similarly, OrcXML is currently broken, but will be fixed before merge.
|
I have posted this before I have corrected the broken things so that review can start ahead of completing the last few things. If you would prefer to wait, go ahead. |
Classes maintain lenience by lifting any captured variables to constructor (partial constructor) parameters. This commit simply disables the closure lenience. Code to support it is still in the token interpreter.
Also clean up TODOs in ClassForms.
This affects the VirtualClock handling more than expected. This introduces a probable bug in the distrib code, but that should be removable since contexts will no longer contain readable closures.
|
Since you're in here, can the ResolvableCollection stuff be collapsed back into ClosureGroup? |
|
It could be yes. However, I don't think we should. It's a extra abstraction that might be useful later and it doesn't confuse the code. |
|
All bugs mentioned above are fixed. The only issue at this point is that these changes introduce a probably bug in TokenReplacement:251. I suspect this problem will go away once distrib has been updated to use the assumption that all Readables are futures and closures are never lenient. |
| @@ -24,7 +24,7 @@ | |||
| * | |||
| * @author dkitchin | |||
| */ | |||
| public abstract class OrcException extends Exception { | |||
| public abstract class OrcException extends RuntimeException { | |||
There was a problem hiding this comment.
Oh, no, keep them checked exceptions, please.
There was a problem hiding this comment.
Why? We are Scala in 99% of the time so they are unchecked anyway.
The reason I changed this is that if they are checked I have to wrap them in the truffle interpreter. I could work around this, but I didn't know of any reason to have them checked.
There was a problem hiding this comment.
For (all the millions of) Java callers to Orc. :)
There was a problem hiding this comment.
I'll look into making it checked. If I cant make it checked without having to wrap and unwrap the at truffle call boundaries I will. Otherwise I will make a TODO (and maybe an issue) and we can revisit it later.
There was a problem hiding this comment.
Actually I will leave it checker here and maybe change it later if I have to.
|
About using Invokers in dOrc. I'd be interested to know how you think you could use them specifically. For instance if you want to be able to serialize them in some cases we should probably add "Take the serializable contract serious, dude" to the documentation so that I don't forget that's important. Some invokers will probably be totally non-serializable (for instance, if they are invokers for JavaScript or native code functions running in Truffle, since those would include references to local truffle, graal, or even native execution state). But we can control that so it seems manageable. |
|
There last two commits should address all the comments you have brought up. |
The initial change introduces a bugs and missing features in a couple of places. These will be fixed before merge into master.