wip #1

ryzokuken · 2018-06-21T18:25:45Z

hashseed · 2018-06-21T18:28:39Z

lib/internal/modules/cjs/loader.js

@@ -122,14 +122,9 @@ var modulePaths = [];
 Module.globalPaths = [];

 Module.wrap = function(script) {
-  return Module.wrapper[0] + script + Module.wrapper[1];
+  return vm.wrapFunction(script);


As discussed, let's rename this. Maybe to getWrappedFunction.

hashseed

Also, the old way offered code caching. We should make this possible too. v8::ScriptCompiler::CompileFunctionInContext actually provides code caching functionality.

hashseed · 2018-06-21T18:30:18Z

src/node_contextify.cc

+  };
+
+  TryCatch try_catch(env->isolate());
+  Environment::ShouldNotAbortOnUncaughtScope no_abort_scope(env);


I don't think you need this. TryCatch catches the exception.

hashseed · 2018-06-21T18:32:52Z

src/node_contextify.cc

+
+  Local<Function> fun = maybe_fun.ToLocalChecked();
+  CHECK(!fun.IsEmpty());
+  CHECK(!try_catch.HasCaught());


This should be written as

Local<Function> fun; if (maybe_fun.ToLocal(&fun)) { // set return value to fun } else { // rethrow exception }

hashseed · 2018-06-21T18:33:23Z

src/node_contextify.cc

+  CHECK(!fun.IsEmpty());
+  CHECK(!try_catch.HasCaught());
+
+  Local<String> result = fun->ToString(context).ToLocalChecked();


This is precisely what we do not want to do. Do not turn the function into a string. We want the function itself.

hashseed · 2018-06-21T18:34:26Z

test/parallel/test-vm-basic.js

+
+// vm.wrapFunction
+{
+  assert.strictEqual(


More tests for

calling the function with arguments and check that the arguments can be used as expected, e.g. can be returned.

another syntax error with random garbage as source

ryzokuken · 2018-06-21T19:28:18Z

Would you expect the user to send Cache as a second argument to the function?

addaleax · 2018-06-22T14:15:59Z

lib/internal/modules/cjs/loader.js

-    displayErrors: true
-  });
+  const func = vm.getWrappedFunction(content, false);
+  var compiledWrapped = func(); // FIXME(ryzokuken): This won't work.


I think you don’t want to call func yet, but instead have compiledWrapper = vm.getWrappedFunction(…) and leave the rest?

originally, compilerWrapper would return a v8::Value that'd be returned as the result of your script, which it'll receive by calling script->Run().

All that I think needs to be done here is somehow calling the function (func->Call()) with the same context (env->context()). All the needed vars (require, __dirname, etc) will be in there, right?

@addaleax on second thought, you're probably right.

addaleax · 2018-06-22T16:53:25Z

src/node_contextify.cc

+    env->module_parameter_module(),
+    env->module_parameter_filename(),
+    env->module_parameter_dirname()
+  };


It might actually be nice if we could pass these in as arguments to for GetWrappedFunction()?

That would be nice, yes. That way this function only provides a primitive to build upon.

@addaleax like... accepting the params array as arguments? Do I fallback to these values as defaults, or do I pass these into the primitive function which requires them?

do I pass these into the primitive function which requires them?

Yes, that. (Assuming “requires” = “accepts as arguments”)

no, by require I mean that you need to pass them in as opposed to optionally specifying them in which case they fallback to defaults.

I think an empty array would make a sensible default here

@addaleax cool. So should I add the said primitive in this PR, or should I follow up with a PR that refactors most of the current code into the primitive, with extra code to handle params as args to the new function would just call the primitive with these params as an arg?

not sure which parts you are referring to by “current code”, but generally, you can use a single PR that contains two logically separate commits:

One that adds the CompileFunctionInContext binding to the vm module and does not touch the module load

One that makes the CJS loader use that binding and doesn’t touch the VM module

Does that help?

ryzokuken · 2018-06-22T17:25:28Z

@addaleax @hashseed while all complications seem to have gone away, ESLint seems to be failing:

Cannot read config file: /Users/ryzokuken/Code/nodejs/node/.eslintrc.js
Error: The "path" argument must be of type string. Received type undefined
TypeError [ERR_INVALID_ARG_TYPE]: Cannot read config file: /Users/ryzokuken/Code/nodejs/node/.eslintrc.js
Error: The "path" argument must be of type string. Received type undefined
    at assertPath (path.js:39:11)
    at Object.dirname (path.js:1270:5)
    at module.exports (<anonymous>:11:34)
    at loadJSConfigFile (<anonymous>:154:16)
    at loadConfigFile (<anonymous>:209:22)
    at loadFromDisk (<anonymous>:495:18)
    at Object.load (<anonymous>:559:20)
    at Config.getLocalConfigHierarchy (<anonymous>:227:44)
    at Config.getConfigHierarchy (<anonymous>:179:43)
    at Config.getConfigVector (<anonymous>:286:21)

any idea how any of my changes could trigger this?

ryzokuken · 2018-06-22T17:51:49Z

We're somehow not helping with the file paths? undefined:752

ryzokuken · 2018-06-22T17:52:40Z

Wait, it's probably because I'm not using the filename option in _compile anymore, but where do I use it? Anywhere we can enter filepaths while compiling functions?

addaleax · 2018-06-22T17:53:19Z

@ryzokuken Maybe look at the existing usage of ScriptOrigin?

(In the end, you probably want a function that resembles the existing ContextifyScript::New() pretty closely, and maybe refactor out some common bits)

ryzokuken · 2018-06-22T18:06:58Z

@addaleax tests run now finally, yay.

hashseed · 2018-06-22T18:25:41Z

src/node_contextify.cc

+    ArrayBuffer::Contents contents = cached_data_buf->Buffer()->GetContents();
+    uint8_t* data = static_cast<uint8_t*>(contents.Data());
+    cached_data = new ScriptCompiler::CachedData(
+      data + cached_data_buf->ByteOffset(), cached_data_buf->ByteLength());


All this can really be done inside the if-block where you checked args[2]. That way you can also define cached_data_buf inside the block.

I did it in this particular fashion to mimic what New did, so I have no preferences here whatsoever. Will do this.

I see. In that case it's alright then.

hashseed · 2018-06-22T18:26:14Z

src/node_contextify.cc

+
+  ScriptCompiler::CompileOptions options;
+  if (source.GetCachedData() == nullptr) {
+    options = ScriptCompiler::kNoCompileOptions;


This option can also be set inside the if-block for args[2].

Same as above, will do.

hashseed · 2018-06-22T18:29:02Z

src/node_contextify.cc

+          env,
+          reinterpret_cast<const char*>(cached_data->data),
+          cached_data->length);
+      fun->Set(env->cached_data_string(), buf.ToLocalChecked());


@addaleax do you have a suggestions as to where the resulting buffer could be better stored?

Alternatively, we don't offer this at all, and expose a separate binding for ScriptCompiler::CreateCodeCacheForFunction similar to how we also recently exposed ScriptCompiler::CreateCodeCache. That way this function gets a bit simpler, but then it would also lose symmetry to ContextifyScript::New.

since the option for ContextifyScript::New is deprecated in my pr i think its best not to mimic it.

@devsnek could you drop me a link to it? Currently it Sets the two on the returned function object itself, which sounds like similar (if not the same) to what we're doing in ContextifyScript::New. Would love to see any alternatives you have in mind.

nodejs#20300

@devsnek cool, this is exactly what @hashseed pointed out, I could do this. That said, it's still just a documentation-level deprecation, right?

well... it will be docs-dep for 10, runtime dep for 11, and removed in 12.

In that case it would be reasonable to not introduce it here in the first place :)

hashseed · 2018-06-22T18:31:29Z

test/parallel/test-vm-basic.js

+// vm.getWrappedFunction
+{
+  assert.strictEqual(
+    vm.getWrappedFunction('console.log("Hello, World!");'),


Can we have a .toString() here to be explicit about what we are comparing here?

definitely. The tests haven't been updated to reflect the latest changes made in the code.

hashseed · 2018-06-22T18:31:37Z

test/parallel/test-vm-basic.js

+    type: SyntaxError,
+    message: 'Unexpected token }'
+  });
+}


I thought you had more tests :)

Yeah, I commented out these as well, for now. Now that the existing tests all pass, will focus on covering as many cases as possible with tests.

hashseed · 2018-06-22T18:33:54Z

(In the end, you probably want a function that resembles the existing ContextifyScript::New() pretty closely, and maybe refactor out some common bits)

This. The new binding should mirror the old code path as closely as possible.

ryzokuken · 2018-06-22T19:00:54Z

The new binding should mirror the old code path as closely as possible.

Agreed. This will definitely be a priority.

BTW, @hashseed @addaleax at this point, all existing tests on master pass, so I personally feel good enough about this, code-wise (we could definitely add more functionality though). LMK whenever you're okay with this going to a public PR.

The TODOs for now are:

Add nice tests
Add appropriate documentation
Use gzemnid, analyze github data, etc to see the usage stats of affected functions, this would help make a nice argument for it once I make the PR
Add support for sandboxes (maybe another PR?)
Make a primitive variant (maybe another PR?)
Make this as similar as possible to New, and then refactor the common bits (maybe another PR?)

addaleax · 2018-06-22T19:56:32Z

@ryzokuken I think the priorities might be another way – making this as similar as possible to New wouldbe #1, and a lot of the rest already follows from that?

devsnek · 2018-06-23T16:45:39Z

src/node_contextify.cc

+  Context::Scope scope(context);
+
+  MaybeLocal<Function> maybe_fun = ScriptCompiler::CompileFunctionInContext(
+      context, &source, 5, params, 0, nullptr, options);


would be nice to also support the extensions param

@devsnek definitely. I had been thinking of shipping in primitives with much more features (sandboxes, extensions, line_offset and comment_offset and what not) in hopefully a subsequent PR but I'm open to adding it in this one.

ryzokuken · 2018-06-24T13:19:54Z

So, I added a few tests that I felt seemed absolutely necessary and made sure that the function works as expected. Let me know if you all have any more test cases in mind.

hashseed

This looks good to me. Aside from design choices for which I'm not the right person.

hashseed · 2018-06-25T17:56:21Z

src/node_contextify.cc

+          env,
+          reinterpret_cast<const char*>(cached_data->data),
+          cached_data->length);
+      fun->Set(env->cached_data_string(), buf.ToLocalChecked());


In that case it would be reasonable to not introduce it here in the first place :)

devsnek · 2018-06-25T18:03:48Z

src/node_contextify.cc

+      Local<Value> val = params_buf->Get(context, n).ToLocalChecked();
+      if (val->IsString()) {
+        params[n] = val.As<String>();
+      } else {


we shouldn't be doing the type checks in c++, just assume they're all strings at this point. (CHECK(val->IsString()) if you must)

I could do this, but I'm not entirely convinced that we shouldn't do any error checking here. Perhaps something on the JS level?

devsnek · 2018-06-25T18:04:05Z

src/node_contextify.cc

+        return;
+      }
+    }
+  } else {


this condition shouldn't be needed, cjs should be a normal consumer of this function

devsnek · 2018-06-25T18:04:27Z

src/node_contextify.cc

+  CHECK(args[1]->IsString());
+  Local<String> filename = args[1].As<String>();
+
+  // Argument 3: line offset


i don't think we really need offsets

I think we’d probably want this for parity with New()

+1 to what @addaleax said. Added the additional args for parity.

devsnek · 2018-06-25T18:05:34Z

src/node_contextify.cc

+
+  // Read params from params buffer
+  size_t params_len;
+  Local<String> *params;


node pointers hang left

Will fix, didn't notice.

devsnek · 2018-06-25T18:06:18Z

src/node_contextify.cc

+  Local<String> *params;
+  if (!params_buf.IsEmpty()) {
+    params_len = params_buf->Length();
+    params = new Local<String>[params_len];


don't forget to free this

Good catch! Will do.

devsnek · 2018-06-25T18:08:34Z

it might be a good idea to open this pr on the node repo to get more feedback

addaleax · 2018-06-25T18:06:24Z

src/node_contextify.cc

+
+  // Read params from params buffer
+  size_t params_len;
+  Local<String> *params;


I think it might be nicer to use a std::vector<Local<String>> – right now, this looks like a memory leak because params is not being deleted anywhere?

(Otherwise, style-nit: left-leaning pointers :))

@addaleax I had been tempted to the that too, but the function needs an array and size value, so why bother putting stuff in a vector just to decompose it later?

Re:delete, it was a oversight and I will delete params appropriately 😅

Because std::vector is basically a cleaner way of what new/delete does, namely it avoids this exact kind of memory leak and is a bit more flexible in general? You can feel free to not do it, but I’d be 90 % sure somebody else would ask for it in the public PR then

addaleax · 2018-06-25T18:06:27Z

src/node_contextify.cc

+    params[1] = env->module_parameter_require();
+    params[2] = env->module_parameter_module();
+    params[3] = env->module_parameter_filename();
+    params[4] = env->module_parameter_dirname();


I still think the default should be an empty array

@devsnek (#1 (comment)) and @addaleax I thought so at first too, but chose otherwise because I thought eternals would be faster?

Anyway, if you're still both +1 to defaulting to an empty array instead, then I think I'm okay with it.

I thought eternals would be faster?

I don’t think it makes much of a difference. If you have an empty default parameter list, you don’t have any extra native streams, that’s also a nice thing.

But more importantly, this sets the API right to be self-contained rather than something with the specific purpose of supporting the module wrapper

addaleax · 2018-06-25T18:08:38Z

lib/vm.js

@@ -294,6 +299,8 @@ module.exports = {
  runInNewContext,
  runInThisContext,
  isContext,
+  getWrappedFunction,
+  getWrappedFunctionGeneric,


I think we only want to have one export of this kind? Maybe this is already implied in the WIP tag here, just want to make sure it doesn’t get lost :)

wanted to expose the generic version in case someone needs it. The getWrappedFunction is a smaller variant with only 2 args and default values.

That said, an options object with defaults sounds better, so I'll probably do that instead.

addaleax · 2018-06-25T18:10:58Z

lib/vm.js

@@ -286,6 +287,10 @@ function runInThisContext(code, options) {
  return createScript(code, options).runInThisContext(options);
 }

+function getWrappedFunction(code, filename) {
+  return getWrappedFunctionGeneric(code, filename, 0, 0, undefined, false);
+}


I would suggest as the public API:

vm.compileFunction(code, params = [], { filename, columnOffset, lineOffset, cachedData, produceCachedData } = {})

Looks good, Will do.

jdalton · 2018-06-25T19:50:57Z

lib/internal/modules/cjs/loader.js

-    lineOffset: 0,
-    displayErrors: true
-  });
+  const compiledWrapper = vm.getWrappedFunction(content, filename);


Would there be a way to have the cake and eat it to? As is the removal of Module.wrap use is a no-go for me since CJS tools rely on it. If there is a way to get the benefit + allow Module.wrap tie-ins that would be rad.

This could be done in a variety of ways. For example using the Module.wrapper[0] and Module.wrapper[1] to be used to compile something internally (waves hands as I'm not familiar with V8 internals). Or having setters on Module.wrapper, Module.wrapper[0], and Module.wrapper[1] to trigger the old non-vm.getWrappedFunction use.

@ryzokuken

@jdalton I didn't remove Module.wrap, nor do I intend to (atleast not in this PR). This PR just changes module._compile to not use it internally, technically intended as a semver-major (because new bindings).

If in case somebody does decide to remove Module.wrap, it will have to go through the standard deprecation process that @devsnek pointed out, thus you could be assured that it won't be going anywhere any soon. Plus we won't do anything that hurts the community, would we? ;)

This is PR addresses a minor inconvenience that has existed since Node's creation so I'm not OK dropping any existing CJS tie-ins here. Module.wrap and Module.wrapper have been used by tooling to customize CJS module construction so I'd be wary of removing their use here. CJS is essentially frozen so I'd urge you to reconsider.

@jdalton Proper syntax error messages that don’t get obfuscated by the added function code are important as well.

If there are specific use cases you’re thinking of, it would be good to know what public API would address those (and possibly whether they can also be addressed by monkey-patching the newly introduced vm method)

@jdalton what specifically makes Module.wrap special to an extent that CJS tools "rely" on it? I'd love to see code that works with Module.wrap but breaks on what we have right now. Aiming for a semver-minor here, I'll be doing whatever I can to make sure no userland code breaks (and technically, it shouldn't, even right now).

I mean, you very well know at this point about all problems that the original function causes and how this will be an improvement, but again, I'd be making sure that nothing breaks in the process and if something does, let me know.

Or having setters on Module.wrapper, Module.wrapper[0], and Module.wrapper[1] to trigger the old non-vm.getWrappedFunction use.

if we must keep using wrapper, this sounds like the best way. i would fire a deprecation warning too

@jdalton again, I'm removing neither Module.wrap nor Module.wrapper here.

If you could point out which CJS modules/tools broke while changing Module._compile internally, I could make sure it wouldn't.

I think what John is referring to isn’t that Module.wrap or Module.wrapper don’t work anymore, it’s that replacing/monkey-patching them doesn’t have the effect of actually changing Node’s behaviour anymore?

Wait, CJS modules are monkey-patching the Module object and we're holding up PRs because it won't allow them to monkey-patch a core module anymore!?

Wait, CJS modules are monkey-patching the Module object and we're holding up PRs because it won't allow them to monkey-patch a core module anymore!?

Yes, it’s the sensible thing to do for us. We have Node’s past lack of proper hook APIs to blame for this, not users who need to get things to work…

CJS is very monkey-patchable. This particular API and functionality has existed since Jan 2011 (so pretty old). Losing the ability to monkey patch in this way is why I'm raising the concern.

addaleax · 2018-06-25T20:52:13Z

Oh, also: The addition of a new vm method is semver-minor, but the module loader change is almost surely going to be considered semver-major.

ryzokuken · 2018-06-25T20:54:18Z

@addaleax no tests fail, and I hope no CITGM runs would fail as well. Why not semver-patch then?

addaleax · 2018-06-25T20:57:10Z

@ryzokuken It’s semver-major because of what John is talking about – it’s going to be a breaking change, effectively.

addaleax · 2018-06-25T20:58:47Z

Btw, one thing that we could do is to implement Module.wrap/Module.wrapper as getter/setter pairs, and fall back to the old mechanism if they are set by userland code. That would allow us to deprecate and eventually them independently, I think?

ryzokuken · 2018-06-25T21:35:29Z

@hashseed @addaleax @devsnek made the changes requested. LMK if I'm still missing something, but hopefully this is ready to go to nodejs/node. Will make a PR tomorrow (hopefully) :)

addaleax

This is basically already an in-depth review, with these points addressed I think this would be good to go

Given the most recent bits of discussion I’d recommend splitting this into two PRs, first taking care of everything except the modules change, so that you can focus on the technical side of things there

addaleax · 2018-06-25T21:38:43Z

lib/vm.js

@@ -26,6 +26,7 @@ const {
  kParsingContext,
  makeContext,
  isContext: _isContext,
+  getWrappedFunction


nit: s/getWrappedFunction/compileFunction/

umm, do you mean I should rename the C++ binding and well? The C++ binding is named getWrappedFunction//GetWrappedFunction while the public API function is called vm.compileFunction.

Yes, that’s what I mean :)

Sure! Will do.

addaleax · 2018-06-25T21:44:07Z

src/node_contextify.cc

+
+  // Read params from params buffer
+  size_t params_len;
+  Local<String>* params;


I’d still really encourage you to use something better than a raw pointer here. There’s a couple options…

std::unique_ptr<Local<String>[]>. If you want to stick with custom allocation, this is what you want – You use it almost the same, but it automatically takes care of de-allocation.

std::vector<Local<String>>. This also avoids dealing with raw pointers, and is probably the most idiomatic thing C++-wise.

MaybeStackBuffer<Local<String>, N> (where N is something like 10 or so). This is a Node.js-specific thing and lacks a lot of the flexibility of built-in container types, but it has the small advantage of avoiding a heap allocation for the common case.

In that case, I'll switch to a vector.

addaleax · 2018-06-25T22:00:29Z

src/node_contextify.cc

+
+    for (int32_t n = 0; n < params_len; n++) {
+      Local<Value> val = params_buf->Get(context, n).ToLocalChecked();
+      CHECK(val->IsString());


Sorry, Github cut off review here because my connection was flaky :/

For one, we don’t currently do any checking that val is actually a string in JS land, so right now passing in an array with non-string elements would crash the process (as far as I can tell)

Then, params_buf->Get(…) can fail if the array element is defined via a getter, so the .ToLocalChecked() may crash too – you probably want something like if (!params_buf->Get(context, n).ToLocal(&val)) return; (but just as a heads up, that would leak memory unless you go with an RAII option for managing params like the ones suggested above)

Yes, this had been troubling me too, and the check was quite different originally. Will do this once I make the switch to vectors.

i'd rather we did the argument handling in jsland and passed just raw arguments like ContextifyScript::New

@devsnek totally doable. Plus then we could make it throw descriptive errors instead of just crashing.

ryzokuken · 2018-06-26T06:42:19Z

@addaleax @devsnek @hashseed done, please take a final look before I make this public (for real). I think all concerns have been addressed.

ryzokuken · 2018-06-26T18:54:56Z

@addaleax @devsnek done. PTAL? Let's get this where it needs to be within like... an hour or so.

addaleax · 2018-06-26T18:59:39Z

src/node_contextify.cc

+    }
+    fun->Set(
+        env->cached_data_produced_string(),
+        Boolean::New(isolate, cached_data_produced));


Both of these should use the non-deprecated overloads of Set() (and ideally also do error handling for these cases)

I didn't check those out, will do this ASAP.

@addaleax I suppose that by non-deprecated you mean ones that take context, key, val instead of plain key, val? How do I know which functions are deprecated?

They are marked V8_DEPRECATE_SOON or V8_DEPRECATED in v8.h :)

Got it, thanks.

addaleax · 2018-06-26T19:00:01Z

src/node_contextify.cc

+  std::vector<Local<String>> params;
+  if (!params_buf.IsEmpty()) {
+    for (uint32_t n = 0; n < params_buf->Length(); n++) {
+      Local<Value> val = params_buf->Get(context, n).ToLocalChecked();


The earlier comment here about ToLocalChecked still stands, btw

Umm, we're checking that each item in params is a string, so that's covered. ToLocalChecked can fail, so I guess we could return if it does. I hope there's no scope of a memory leak anymore, right?

Yes, we tested that it’s a string (although a sufficiently evil getter could also return a string while typechecking and then later return something else)

What I meant was that if it’s a getter, it can always fail, so we should return back to JS if it does

I hope there's no scope of a memory leak anymore, right?

I don’t think so, no. :)

ryzokuken · 2018-06-26T20:27:37Z

@addaleax I guess this does it?

Reverting this enables us to provide slower, but longer-lasting replacements for the deprecated APIs. Original commit message: Put back deleted V8_DEPRECATE_SOON methods This partially reverts https://chromium-review.googlesource.com/c/v8/v8/+/1177861, which deleted many V8_DEPRECATE_SOON methods rather than moving them to V8_DEPRECATED first. This puts them back and marks them V8_DEPRECATED. Note V8_DEPRECATED that were deleted in the same CL stay deleted. NOTRY=true NOPRESUBMIT=true NOTREECHECKS=true Bug: v8:7786, v8:8240 Cq-Include-Trybots: luci.chromium.try:linux_chromium_rel_ng Change-Id: I00330036d957f98dab403465b25e30d8382aac22 Reviewed-on: https://chromium-review.googlesource.com/1251422 Commit-Queue: Dan Elphick <[email protected]> Reviewed-by: Yang Guo <[email protected]> Reviewed-by: Michael Hablich <[email protected]> Cr-Commit-Position: refs/branch-heads/7.0@{nodejs#49} Cr-Branched-From: 6e2adae6f7f8e891cfd01f3280482b20590427a6-refs/heads/7.0.276@{#1} Cr-Branched-From: bc08a8624cbbea7a2d30071472bc73ad9544eadf-refs/heads/master@{nodejs#55424} Refs: v8/v8@9136dd8 Refs: nodejs#23122 PR-URL: nodejs#23158 Reviewed-By: Yang Guo <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

hashseed reviewed Jun 21, 2018

View reviewed changes

ryzokuken force-pushed the module-17396 branch from 9841df3 to a63861f Compare June 21, 2018 19:16

addaleax reviewed Jun 22, 2018

View reviewed changes

ryzokuken force-pushed the module-17396 branch 2 times, most recently from 9c0cfb8 to 2accbab Compare June 22, 2018 16:49

addaleax reviewed Jun 22, 2018

View reviewed changes

hashseed reviewed Jun 22, 2018

View reviewed changes

devsnek reviewed Jun 23, 2018

View reviewed changes

hashseed approved these changes Jun 25, 2018

View reviewed changes

devsnek suggested changes Jun 25, 2018

View reviewed changes

addaleax reviewed Jun 25, 2018

View reviewed changes

jdalton reviewed Jun 25, 2018

View reviewed changes

addaleax reviewed Jun 25, 2018

View reviewed changes

addaleax reviewed Jun 26, 2018

View reviewed changes

ryzokuken added 18 commits June 28, 2018 11:34

wip

ba9f1c8

wip2

ef24aaa

small-wip3

63b780d

small-wip4

1e22bcf

re-add wrapper

e9b1ba8

wip5

ac1000f

wip6

f5f6bb8

add basic tests

97e6bd4

make tests pass

46cc9be

step everything up a bit?

b5c534e

add args for params

0ac8d78

fix lint failure

7521df0

make everything better

19934f2

Add and expose vm.compileFunction

7c037d2

Remove eternals

cedba7c

Final touches

7603fed

use a vector

d306efd

Rename function to compileFunction

40c2296

ryzokuken force-pushed the module-17396 branch from e62890a to 40c2296 Compare June 28, 2018 06:04

ryzokuken closed this Jul 8, 2018

wip #1

wip #1

Conversation

ryzokuken commented Jun 21, 2018

Choose a reason for hiding this comment

hashseed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Jun 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Jun 22, 2018

ryzokuken commented Jun 22, 2018

ryzokuken commented Jun 22, 2018

addaleax commented Jun 22, 2018

ryzokuken commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Jun 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashseed commented Jun 22, 2018

ryzokuken commented Jun 22, 2018 • edited Loading

addaleax commented Jun 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Jun 24, 2018

hashseed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken Jun 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Jun 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Jun 23, 2018 •

edited

Loading

ryzokuken commented Jun 22, 2018 •

edited

Loading

ryzokuken Jun 25, 2018 •

edited

Loading

jdalton Jun 25, 2018 •

edited

Loading