vm: add bindings for v8::CompileFunctionInContext #21571

ryzokuken · 2018-06-28T06:43:07Z

Adds a method compileFunction to the vm module, which serves as a
binding for v8::CompileFunctionInContext with appropriate args for
specifying the details, and provide params for the wrapper.

Eventually, we would be changing Module._compile to use this internally
over the standard Module.wrap

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

TODO

Add documentation
Add more test cases? (Please suggest any test cases you have in mind)

/cc @nodejs/vm @nodejs/v8 @hashseed

nodejs-github-bot · 2018-06-28T06:43:08Z

@ryzokuken build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/170/pipeline

addaleax · 2018-06-28T06:47:02Z

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();


Can you return if Get() returns an empty MaybeLocal?

addaleax · 2018-06-28T06:47:19Z

src/node_contextify.cc

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


Can you use the non-deprecated overloads of Set()?

src/node_contextify.cc

+
+  // Argument 4: column offset
+  CHECK(args[3]->IsNumber());
+  Local<Integer> column_offset = args[3].As<Integer>();


devsnek · 2018-06-28T07:35:19Z

src/node_contextify.cc

+          env->cached_data_string(),
+          buf.ToLocalChecked()).IsNothing()) return;
+    }
+    if (fun->Set(


what if the return value was something like { function, codeCache }

That could be rad, but I'm supposing that most people would just be expecting the function or would disable the cache and then we'll be returning { function }, which doesn't look super good?

Like mentioned previously, let's not have this at all, and expose ScriptCompiler::CreateCodeCacheForFunction in a separate binding like we already do for ScriptCompiler::CreateCodeCache.

devsnek · 2018-06-28T07:35:24Z

src/node_contextify.cc

+
+  // Argument 3: line offset
+  CHECK(args[2]->IsNumber());
+  Local<Integer> line_offset = args[2].As<Integer>();


have it been tested these offsets actually do anything for compiling functions?

No idea, my guess is that we set them to 0 for like... 99% cases, but I put them in just for parity with ContextifyScript::New.

well in ContextifyScript the entire source can be offset for dealing with source positions in html bodies and such. if they don't do anything here it would be best to remove them. (easy test is offset by a bunch and make the body have a syntax error, then read the column offset in the stack)

did you ever verify this?

I didn't, sorry.

@addaleax @hashseed is it worth putting these in just for the sake of parity?

I think this could be useful.

One way to test this is to throw an exception ans check whether the exception position is affected by the offsets.

It indeed works. Will add tests.

ryzokuken · 2018-06-28T07:40:55Z

@addaleax added typechecking for code and params do we need to check the types of all the options as well?

addaleax · 2018-06-28T07:42:22Z

@ryzokuken If you have a CHECK() in your native code, you should make sure that that check cannot be reached from userland as well as that is possible (so, yes)

hashseed · 2018-06-28T07:43:32Z

Should we discuss the option of splitting code caching into a separate function entirely? There is no reason to bundle it with compilation aside from symmetry with ContextifyScript::New, where the code caching option is going to be deprecated.

ryzokuken · 2018-06-28T07:44:03Z

Cool, will add the checks in JS-Land for all options as well, in that case.

ryzokuken · 2018-06-28T07:44:47Z

Should we discuss the option of splitting code caching into a separate function entirely? There is no reason to bundle it with compilation aside from symmetry with ContextifyScript::New, where the code caching option is going to be deprecated.

Definitely. I think that's what we're going towards for New anyway, right?

devsnek · 2018-06-28T07:45:32Z

@hashseed what happens if the target of CreateCodeCacheForFunction wasn't created by CompileFunction? is there a way to tell the difference between invalid target and other reasons cache would fail?

ryzokuken · 2018-06-28T18:47:27Z

lib/vm.js

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

+function compileFunction(code, params, options) {
+  const defaults = {


That seemed a better option, true, but now I'm also using defaults for error checking, so I guess it's good enough?

cjihrig · 2018-06-28T13:50:16Z

lib/vm.js

+  if (typeof code !== 'string') {
+    throw new ERR_INVALID_ARG_TYPE('code', 'string', code);
+  }
+  if (Array.isArray(params)) {


You can save a level of indentation here if you check for !Array.isArray(params) with a throw, and drop the else.

ryzokuken · 2018-06-28T18:46:07Z

test/parallel/test-vm-basic.js

@@ -128,3 +128,30 @@ const vm = require('vm');
             'Received type object'
  });
 });
+
+// vm.compileFunction


ryzokuken · 2018-07-02T16:39:45Z

All: Please take a final look at this, will be landing this tomorrow morning.

ryzokuken · 2018-07-02T16:40:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15705/

ryzokuken · 2018-07-02T20:00:53Z

@devsnek @hashseed what about now?

src/node_contextify.cc

+
+  if (produce_cached_data) {
+    const ScriptCompiler::CachedData* cached_data =
+      ScriptCompiler::CreateCodeCacheForFunction(fun, code);


TimothyGu

Sorry about the late review...

TimothyGu · 2018-07-03T00:01:17Z

lib/vm.js

+    filename,
+    columnOffset,
+    lineOffset,
+    cachedData,


This is not type-checked.

Could you elaborate? It is checked if cachedData is an Object or not, but it's not checked in it's a Uint8Array. Do you propose I add checking for the exact type? I think I should do that, but couldn't find instances of the same in the codebase.

Do you propose I add checking for the exact type?

Yes.

I think I should do that, but couldn't find instances of the same in the codebase.

Because the entire (or at least most of) codebase is now free of Uint8Array-only functions. You could use isUint8Array() in internal/util/types.js for this purpose.

TimothyGu · 2018-07-03T00:01:51Z

lib/vm.js

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

+function compileFunction(code, params, options) {
+  const defaults = {


This object should be lifted out of the function as it's never changed. Optionally it can also be Object.freeze()d.

Of course, defaults object won't be necessary if you instead use destructuring with defaults as I did in #21571 (comment).

That's exactly what @targos suggested, but then I'm also using the same object for type-checking. Would transition to a hard-coded types approach instead of the inferred approach as used here, it sounds better in this case anyway.

Would transition to a hard-coded types approach instead of the inferred approach as used here

Yes that is exactly what I'm suggesting in #21571 (comment).

TimothyGu · 2018-07-03T00:03:19Z

src/node_contextify.cc

+  // Argument 5: cached data (optional)
+  Local<Uint8Array> cached_data_buf;
+  if (!args[4]->IsUndefined()) {
+    CHECK(args[4]->IsUint8Array());


This should accept all ArrayBuffer view types, and ArrayBuffer itself, consistent with other newer APIs. Hint: use Buffer::HasInstance()

I am kinda split if I should use Buffers or stick with what we have, also because it's consistent with Script::New. @hashseed @addaleax would love to hear your thoughts on this.

I’d stick with consistency with Script::New, and maybe relax this in a later PR for both of them?

Sounds great to me. @TimothyGu wdyt?

TimothyGu · 2018-07-03T00:07:25Z

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());


Buffer methods should simplify this quite a bit, as they take care of byte offsets.

This hasn't been fixed yet. Note, you don't need to support all types of ArrayBufferView to use Buffer methods. In fact you can do a direct switch of data + cached_data_buf->ByteOffset() by Buffer::Data(cached_data_buf) and cached_data_buf->ByteLength() by Buffer::Length(cached_data_buf).

I realized that, and had been planning to change these in both CompileFunction and Script::New in a quick subsequent PR. I could make one right now, if you'd like.

Or would you still prefer that I make the switch in this one and just change Script::New in the other PR?

Nah, do as you planned :)

TimothyGu · 2018-07-03T00:09:23Z

lib/vm.js

+  }
+
+  if (options !== undefined) {
+    for (const key in options) {


I'd prefer not using for-in loops for checking types... it doesn't scale well when we want to add new options with odd types.

Any suggestions on what I should use instead? I think I should with Object.keys(options).forEach(...), but I not a lot of alternatives come to mind.

Just do

const { optionA = 'default value', optionB, optionC } = options; if (typeof optionA !== 'string') { throw ... } if (optionB !== undefined && !isContext(optionB)) { throw ... } validateInteger(optionC);

TimothyGu · 2018-07-03T00:09:55Z

src/node_contextify.cc

+
+  // Argument 3: line offset
+  CHECK(args[2]->IsNumber());
+  Local<Integer> line_offset = args[2].As<Integer>();


The Number type is indeed checked in JavaScript, but not that the number is an integer. That should be added.

Agreed, will throw in Number.isInteger checks.

BTW you should use validateInteger() in internal/validators.js.

ryzokuken · 2018-07-08T17:18:25Z

Testing the new changes locally, let's merge it in the next few days and proceed?

TODOs for subsequent PRs:

Move to a more general Buffer both in CompileFunction and Script::New
Move the caching mechanism to a separate function

/cc @addaleax @hashseed @TimothyGu @devsnek

ryzokuken · 2018-07-08T19:42:45Z

CI: https://ci.nodejs.org/job/node-test-pull-request/15761/

TimothyGu · 2018-07-09T01:33:57Z

lib/vm.js

+  }
+  if (parsingContext !== undefined) {
+    try {
+      isContext(parsingContext);


isContext returns a boolean for objects. You probably want something like if (typeof parsingContext !== 'object' || parsingContext === null || !isContext(parsingContext)).

I tried exactly that, but apparently isContext throws by itself?

function isContext(sandbox) { if (typeof sandbox !== 'object' || sandbox === null) { throw new ERR_INVALID_ARG_TYPE('sandbox', 'Object', sandbox); } return _isContext(sandbox); }

Yeah it throws for non-objects, but returns a boolean for objects. Check out the implementation for _isContext:

node/src/node_contextify.cc

Lines 261 to 271 in 978d89f

void ContextifyContext::IsContext(const FunctionCallbackInfo<Value>& args) {

Environment* env = Environment::GetCurrent(args);

CHECK(args[0]->IsObject());

Local<Object> sandbox = args[0].As<Object>();

Maybe<bool> result =

sandbox->HasPrivate(env->context(),

env->contextify_context_private_symbol());

args.GetReturnValue().Set(result.FromJust());

}

That's the thing: it throws for null as well as non-objects, and I'm testing for null, which complicates things. Isn't null an object too? It shouldn't fail the CHECK, right?

TimothyGu · 2018-07-09T02:17:45Z

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());


This hasn't been fixed yet. Note, you don't need to support all types of ArrayBufferView to use Buffer methods. In fact you can do a direct switch of data + cached_data_buf->ByteOffset() by Buffer::Data(cached_data_buf) and cached_data_buf->ByteLength() by Buffer::Length(cached_data_buf).

ryzokuken · 2018-07-11T03:14:27Z

@TimothyGu PTAL

jdalton · 2018-07-11T15:25:04Z

lib/vm.js

+      parsingContext
+    );
+  }
+


👆 All that validation stuff is super clean. I dig it 👍

jdalton · 2018-07-11T15:26:04Z

lib/vm.js

+      }
+    });
+  }
+


👆 I know many internals try to avoid prototypal methods directly (opting to create generic forms). Should this be done for forEach use?

Update:

Same with isArray (e.g. const { isArray } = Array)

We've generally tried to avoid using forEach within core functions because it's an order of magnitude slower than a regular for-loop. Even if this is not expected to be a hot function, I think I'd rather see this as a regular for-loop.

if we're taking the performance approach, forEach has actually become faster in the common case than a for loop (in V8 anyway).

👆 \cc @bmeurer

Whatever the outcome of this discussion, please let’s not hold up the PR over this – this isn’t hot code (or at least the cost of actual parsing + compilation is likely going to overshadow any choice of this kind), so I think it’s fine to go with whatever the PR author considers most readable.

ryzokuken · 2018-07-11T19:11:55Z

Optimistic CI Run: https://ci.nodejs.org/job/node-test-pull-request/15811/ (last couple of runs failed)

targos · 2018-07-11T20:19:48Z

Code LGTM. This just needs to be documented.

jasnell · 2018-08-12T16:35:25Z

Re-run custom-suites CI: https://ci.nodejs.org/job/node-test-commit-custom-suites/627/

jasnell · 2018-08-12T16:46:01Z

@ryzokuken ... can I ask you to squash the fixup commits down? This doesn't land cleanly with all of the edits.

ryzokuken · 2018-08-12T18:11:00Z

@jasnell totally! I had been waiting for everyone to approve this before squashing. Gimme a minute.

ryzokuken · 2018-08-12T18:15:15Z

@jasnell done! PTAL.

jasnell · 2018-08-12T18:17:06Z

New CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16389/

jasnell · 2018-08-24T18:48:42Z

This needs another quick rebase then I can run CI again.

ryzokuken · 2018-08-24T19:49:22Z

@jasnell on it!

Adds a method compileFunction to the vm module, which serves as a binding for v8::CompileFunctionInContext with appropriate args for specifying the details, and provide params for the wrapper. Eventually, we would be changing Module._compile to use this internally over the standard Module.wrap

ryzokuken · 2018-08-24T19:54:32Z

@jasnell That should do it. Thanks!

addaleax · 2018-08-25T00:05:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16744/

targos · 2018-08-28T12:21:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/16827/
Windows rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/20371/

targos · 2018-08-29T14:42:00Z

Landed in 1abbe0a 🎉

Adds a method compileFunction to the vm module, which serves as a binding for v8::CompileFunctionInContext with appropriate args for specifying the details, and provide params for the wrapper. Eventually, we would be changing Module._compile to use this internally over the standard Module.wrap PR-URL: nodejs#21571 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>

Adds a method compileFunction to the vm module, which serves as a binding for v8::CompileFunctionInContext with appropriate args for specifying the details, and provide params for the wrapper. Eventually, we would be changing Module._compile to use this internally over the standard Module.wrap PR-URL: #21571 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22394 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22392 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

Adds a method compileFunction to the vm module, which serves as a binding for v8::CompileFunctionInContext with appropriate args for specifying the details, and provide params for the wrapper. Eventually, we would be changing Module._compile to use this internally over the standard Module.wrap PR-URL: #21571 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

ryzokuken self-assigned this Jun 28, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 28, 2018

addaleax reviewed Jun 28, 2018

View reviewed changes

ryzokuken mentioned this pull request Jun 28, 2018

module: use compileFunction over Module.wrap #21573

Closed

4 tasks

devsnek reviewed Jun 28, 2018

View reviewed changes

targos reviewed Jun 28, 2018

View reviewed changes

cjihrig reviewed Jun 28, 2018

View reviewed changes

addaleax approved these changes Jul 2, 2018

View reviewed changes

src/node_contextify.cc Outdated

if (produce_cached_data) {

const ScriptCompiler::CachedData* cached_data =

ScriptCompiler::CreateCodeCacheForFunction(fun, code);

This comment was marked as resolved.

Sign in to view

TimothyGu added the vm Issues and PRs related to the vm subsystem. label Jul 3, 2018

TimothyGu reviewed Jul 3, 2018

View reviewed changes

TimothyGu reviewed Jul 9, 2018

View reviewed changes

TimothyGu approved these changes Jul 11, 2018

View reviewed changes

jdalton reviewed Jul 11, 2018

View reviewed changes

targos mentioned this pull request Aug 6, 2018

src: perform integrity checks on built-in code cache #22152

Closed

3 tasks

ryzokuken force-pushed the vm-compile-function branch from 9f8bad6 to a5f85ad Compare August 12, 2018 18:13

ryzokuken force-pushed the vm-compile-function branch from a5f85ad to bbd542e Compare August 24, 2018 19:54

targos closed this Aug 29, 2018

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

ryzokuken mentioned this pull request Oct 1, 2018

docs: parsingContext option for vm.compileFunction is a little unclear #23194

Closed

	void ContextifyContext::IsContext(const FunctionCallbackInfo<Value>& args) {
	Environment* env = Environment::GetCurrent(args);

	CHECK(args[0]->IsObject());
	Local<Object> sandbox = args[0].As<Object>();

	Maybe<bool> result =
	sandbox->HasPrivate(env->context(),
	env->contextify_context_private_symbol());
	args.GetReturnValue().Set(result.FromJust());
	}

vm: add bindings for v8::CompileFunctionInContext #21571

vm: add bindings for v8::CompileFunctionInContext #21571

Conversation

ryzokuken commented Jun 28, 2018 • edited

Checklist

TODO

nodejs-github-bot commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

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 28, 2018 • edited

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 28, 2018

addaleax commented Jun 28, 2018

hashseed commented Jun 28, 2018

ryzokuken commented Jun 28, 2018

ryzokuken commented Jun 28, 2018

devsnek commented Jun 28, 2018 • edited

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

ryzokuken commented Jul 2, 2018

ryzokuken commented Jul 2, 2018

ryzokuken commented Jul 2, 2018

This comment was marked as resolved.

TimothyGu 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

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

Choose a reason for hiding this comment

TimothyGu Jul 4, 2018 • edited

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 Jul 8, 2018

ryzokuken commented Jul 8, 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

ryzokuken commented Jul 11, 2018

Choose a reason for hiding this comment

jdalton Jul 11, 2018 • edited

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 Jul 11, 2018 • edited

targos commented Jul 11, 2018

ryzokuken commented Jun 28, 2018 •

edited

devsnek Jun 28, 2018 •

edited

devsnek commented Jun 28, 2018 •

edited

TimothyGu Jul 4, 2018 •

edited

jdalton Jul 11, 2018 •

edited

ryzokuken commented Jul 11, 2018 •

edited

targos commented Aug 28, 2018 •

edited