src: refactor options parsing #22392

addaleax · 2018-08-18T22:52:16Z

In case anybody was wondering what I’ve been up to this week. 😸

This is a major refactor of our Node’s parser. See node_options.cc
for how it is used, and node_options-inl.h for the bulk
of its implementation.

Unfortunately, the implementation has come to have some
complexity, in order to meet the following goals:

Make it easy to use for defining or changing options.
Keep it (mostly) backwards-compatible.
- No tests were harmed as part of this commit.
Be as consistent as possible.
- In particular, options can now generally accept arguments
  through both --foo=bar notation and --foo bar notation.
  We were previously very inconsistent on this point.
  (Labelling semver-minor because of this).
Separate into different levels of scope, namely
per-process (global), per-Isolate and per-Environment
(+ debug options).
Allow programmatic accessibility in the future.
- This includes a possible expansion for --help output.

This commit also leaves a number of TODO comments, mostly for
improving consistency even more (possibly with having to modify
tests), improving embedder support, as well as removing pieces of
exposed configuration variables that should never have become
part of the public API but unfortunately are at this point.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Fyi @boneskull, you probably want to be aware of this because of #19335. :)

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point.

nodejs-github-bot · 2018-08-18T22:52:17Z

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

refack · 2018-08-18T23:04:26Z

src/node_config.cc

@@ -72,35 +72,38 @@ static void Initialize(Local<Object> target,
  READONLY_BOOLEAN_PROPERTY("hasTracing");
 #endif

-  READONLY_STRING_PROPERTY(target, "icuDataDir", icu_data_dir);
+  // TODO(addaleax): This seems to be an unused, private API. Remove it?


I remember @srl295 saying this might be used by embedders.

It’s only accessible through process.binding('config'), so it’s still going to have to change somehow, assuming we push through with the process.binding() deprecations…

refack

I love it! Way way way overdue refactoring.
But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

Downgrading to comment

refack · 2018-08-18T23:25:21Z

src/node_options.h

+  // These methods add a single option to the parser. Optionally, it can be
+  // specified whether the option should be allowed from environment variable
+  // sources (i.e. NODE_OPTIONS).
+  void AddOption(const std::string& name,


Why can't these be templates?

The way this is done is because we use it to infer the info.type value. We could do something with templates but I think it would mean we’d still use a list of template specializations?

addaleax · 2018-08-18T23:29:51Z

But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

I don’t really understand this comment, can you explain or give examples?

refack · 2018-08-18T23:34:06Z

src/node_options.cc

+
+namespace node {
+
+DebugOptionsParser::DebugOptionsParser() {


AFAIR this could be merged with EnvironmentOptionsParser. It was encapsulated as in preparation to something like this PR.

Yes, this could be merged. As you are saying, it helps with encapsulation, so I don’t mind it either way.

I would add a comment stating this, just so that future generations won't assume it's something special.

Sounds good, done! (3c9b03a)

jasnell · 2018-08-18T23:50:41Z

Fantastic start on this. One the things I would really like to see is a quick way of determining if the options have changed from one run to another (essentially a hash value calculated from the options values)

addaleax · 2018-08-18T23:52:30Z

@jasnell Just so I understand it, something that one could get today Hash(JSON.stringify(process.execArgv)) except that the values of all options are taken into account?

jasnell · 2018-08-18T23:56:00Z

Yes and no. Env vars also need to be taken into consideration and input should be normalized so that order doesn't matter (except in those handful of cases where it does, of course). It also needs to take into consideration default values that may happen to change.

refack · 2018-08-18T23:59:05Z

But I don't like that OptionsParser is dynamically extensible. AFAIK it's a well established wheel, so either we assume an existing implementation, or make it static.

I don’t really understand this comment, can you explain or give examples?

I mean implementing a generic agrv parser. I don't have enough production experience with any specific library but https://github.com/search?l=C%2B%2B&q=argument+parser&type=Repositories show afew with > 100 stars.
If we don't want any of those I would have liked something static. So instead of:

AddOption("--inspect-port", &DebugOptions::host_port,
            kAllowedInEnvironment);
AddAlias("--debug-port", "--inspect-port");
AddOption("--inspect", &DebugOptions::inspector_enabled,
            kAllowedInEnvironment);
AddAlias("--inspect=", { "--inspect-port", "--inspect" });
AddOption("--debug", &DebugOptions::deprecated_debug);
AddAlias("--debug=", { "--inspect-port", "--debug" });

we would have something like

Option<"inspect-port", kAllowedInEnvironment> host_port;
Alias<"inspect-port"> debug-port;

that might requires reflection, or c'tors with side effects, or macros...

So in that sense your implementation is quite minimal, it's just that most of that information is known in compile time, so there no need to delegate it to run time.

addaleax · 2018-08-19T00:03:47Z

I mean implementing a generic agrv parser. I don't have enough production experience with any specific library but https://github.com/search?l=C%2B%2B&q=argument+parser&type=Repositories show afew with > 100 stars.

I think that’s not doable given how weird some of our options are (think debug flags or --print/--eval), at least as long as we’re going for backwards compatibility…

So in that sense your implementation is quite minimal, it's just that most of that information is known in compile time, so there no need to delegate it to run time.

I agree that there is unnecessary runtime overhead here that I’d like to get down a bit, yes. 👍

refack · 2018-08-20T00:12:22Z

src/node_options-inl.h

+                                       bool Options::* field,
+                                       OptionEnvvarSettings env_setting) {
+  options_.emplace(name, OptionInfo {
+    kBoolean,


Just thinking out loud... (It is an interesting problem after all).
How about instead on a const you put a pointer to the conversion function. That way you make these Templates specializations on the typeof(field), and eliminate the switch in L385.

Had a few more "mind itches". I'll try to write up a draft.

Had a few more "mind itches". I'll try to write up a draft.

Sounds good, looking forward to it!

mcollina

LGTM.

Can you also do a CITGM run?

addaleax · 2018-08-20T13:13:57Z

Yup, sounds like a good idea:

CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1517/
CITGM (master): https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1516/

boneskull · 2018-08-20T21:02:19Z

@addaleax I don't think this conflicts too badly with #19335, but I have need to separate "options passed through to V8" (e.g., --abort-on-uncaught-exception) from "Node.js native options".

How can I use this API to get all flags which fall into either bucket?

addaleax · 2018-08-20T21:27:15Z

@boneskull If this PR stays in its current shape, it shouldn’t be too hard – we could go through the options_ map of the parser, and take each entry where the type entry is kV8Option … that would currently yield --abort_on_uncaught_exception, --max_old_space_size, --perf_basic_prof, --perf_prof, --stack_trace_limit. Does that sound about right?

addaleax · 2018-08-21T23:24:27Z

@refack Ping … anything you’d definitely like to see changed before merging this? It would be nice to get this in and work on expanding it (programmatic accessibility/help texts). :)

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

refack · 2018-08-21T23:42:51Z

anything you’d definitely like to see changed before merging this? It would be nice to get this in and work on expanding it (programmatic accessibility/help texts). :)

No reason to wait. If I find the time we could iterate later.

refack · 2018-08-22T00:35:46Z

~~Why on macOS does gcc complains about std::atoll? We run it with -std=gnu++1y which I thought was >= c++14 > c++11~~
Ohh it's clang...

19:29:53 ../src/node_options-inl.h:392:33: error: no member named 'atoll' in namespace 'std'; did you mean simply 'atoll'?
19:29:53             ->Lookup(options) = std::atoll(value.c_str());
19:29:53                                 ^~~~~~~~~~
19:29:53                                 atoll
19:29:53 /usr/include/stdlib.h:136:3: note: 'atoll' declared here
19:29:53          atoll(const char *);
19:29:53          ^
19:29:53 ../src/node_options.cc:187:3: error: use of undeclared identifier 'errno'
19:29:53   errno = 0;
19:29:53   ^
19:29:53 ../src/node_options.cc:189:7: error: use of undeclared identifier 'errno'
19:29:53   if (errno != 0 || *endptr != '\0'||
19:29:53       ^

addaleax · 2018-08-22T00:48:26Z

@refack I think it might just be that stdlib.h is included but cstdlib isn’t…

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

refack · 2018-08-22T01:21:38Z

src/node_options.cc

+
+inline int ParseAndValidatePort(const std::string& port, std::string* error) {
+  char* endptr;
+  errno = 0;


Isn't this lint? How did this compile before? Ahh the wonder of C++ compilers...
And in the next line could we switch to strtoul?

Lint? It’s just a missing include, I assume… and, yes, I think we could switch, but it shouldn’t make any difference?

It’s just a missing include

ahh it's defined in errno.h (that's a POSIX mechanism). Without that context it does look like an undefined var (who's value magicly changes).

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: #22392 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

refack · 2018-08-23T17:03:41Z

@addaleax PTAL at #22484. They found a hole in out CI matrix.

I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: nodejs#22526 Refs: nodejs#22392

I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: nodejs#22392 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: #22392 Backport-PR-URL: #22558 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: #22392 Backport-PR-URL: #22558 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

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

This is a major refactor of our Node’s parser. See `node_options.cc` for how it is used, and `node_options-inl.h` for the bulk of its implementation. Unfortunately, the implementation has come to have some complexity, in order to meet the following goals: - Make it easy to *use* for defining or changing options. - Keep it (mostly) backwards-compatible. - No tests were harmed as part of this commit. - Be as consistent as possible. - In particular, options can now generally accept arguments through both `--foo=bar` notation and `--foo bar` notation. We were previously very inconsistent on this point. - Separate into different levels of scope, namely per-process (global), per-Isolate and per-Environment (+ debug options). - Allow programmatic accessibility in the future. - This includes a possible expansion for `--help` output. This commit also leaves a number of `TODO` comments, mostly for improving consistency even more (possibly with having to modify tests), improving embedder support, as well as removing pieces of exposed configuration variables that should never have become part of the public API but unfortunately are at this point. PR-URL: #22392 Backport-PR-URL: #22558 Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]>

I, uhm, might have messed up by using a `substr(start, end)` signature when `std::string` actually uses `substr(start, len)`. Fix that. Fixes: #22526 Refs: #22392 PR-URL: #22529 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]>

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

addaleax added semver-minor PRs that contain new features and should be released in the next minor version. cli Issues and PRs related to the Node.js command line interface. labels Aug 18, 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 Aug 18, 2018

refack suggested changes Aug 18, 2018

View reviewed changes

refack previously requested changes Aug 18, 2018

View reviewed changes

refack reviewed Aug 18, 2018

View reviewed changes

fixup! src: refactor options parsing

3c9b03a

refack reviewed Aug 20, 2018

View reviewed changes

mcollina approved these changes Aug 20, 2018

View reviewed changes

refack approved these changes Aug 21, 2018

View reviewed changes

fixup! src: refactor options parsing

9b811b3

refack reviewed Aug 22, 2018

View reviewed changes

addaleax deleted the options branch August 22, 2018 21:41

BridgeAR mentioned this pull request Aug 25, 2018

NODE_OPTIONS do not work properly anymore #22526

Closed

addaleax mentioned this pull request Aug 26, 2018

src: fix NODE_OPTIONS parsing bug #22529

Closed

3 tasks

addaleax mentioned this pull request Aug 27, 2018

[v10.x] src: refactor options parsing #22558

Closed

targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 28, 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

addaleax mentioned this pull request Sep 10, 2018

--prof-process CLI args no longer work in 10.10.0 #22786

Closed

demurgos mentioned this pull request Nov 25, 2018

End of CLI options -- is now part of process.execArgv (regression in Node 10.10) #24647

Closed

ckerr mentioned this pull request Dec 6, 2023

fix: wrong default port in docs electron/electron#40665

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: refactor options parsing #22392

src: refactor options parsing #22392

addaleax commented Aug 18, 2018

nodejs-github-bot commented Aug 18, 2018

refack Aug 18, 2018

addaleax Aug 18, 2018

refack left a comment •

edited

Loading

refack Aug 18, 2018

addaleax Aug 18, 2018

addaleax commented Aug 18, 2018

refack Aug 18, 2018

addaleax Aug 18, 2018

refack Aug 18, 2018

addaleax Aug 18, 2018

jasnell commented Aug 18, 2018

addaleax commented Aug 18, 2018

jasnell commented Aug 18, 2018

refack commented Aug 18, 2018

addaleax commented Aug 19, 2018

refack Aug 20, 2018

refack Aug 20, 2018

addaleax Aug 20, 2018

mcollina left a comment

addaleax commented Aug 20, 2018

boneskull commented Aug 20, 2018

addaleax commented Aug 20, 2018

addaleax commented Aug 21, 2018

refack commented Aug 21, 2018

refack commented Aug 22, 2018 •

edited

Loading

addaleax commented Aug 22, 2018

refack Aug 22, 2018

addaleax Aug 22, 2018

refack Aug 22, 2018

refack commented Aug 23, 2018

src: refactor options parsing #22392

src: refactor options parsing #22392

Conversation

addaleax commented Aug 18, 2018

Checklist

nodejs-github-bot commented Aug 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Aug 18, 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

jasnell commented Aug 18, 2018

addaleax commented Aug 18, 2018

jasnell commented Aug 18, 2018

refack commented Aug 18, 2018

addaleax commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

addaleax commented Aug 20, 2018

boneskull commented Aug 20, 2018

addaleax commented Aug 20, 2018

addaleax commented Aug 21, 2018

refack commented Aug 21, 2018

refack commented Aug 22, 2018 • edited Loading

addaleax commented Aug 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Aug 23, 2018

refack left a comment •

edited

Loading

refack commented Aug 22, 2018 •

edited

Loading