src: improve SSL version extraction logic #23050

gireeshpunathil · 2018-09-24T09:03:16Z

The openssl version as defined in ssl libraries is complex. The current logic to extract the major.minor.pacth format uses C semantics to loop through the text and search for specific patterns. Use C++ string to tidy it up.

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

nodejs-github-bot · 2018-09-24T09:03:17Z

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

addaleax

Thanks!

Commit message also has a typo (SSL) :)

addaleax · 2018-09-24T09:05:35Z

src/node.cc

-      }
-    }
+  {
+    // sample opnssl version stirng format


typo: string

gireeshpunathil · 2018-09-24T09:38:51Z

@addaleax - thanks, fixed both.

richardlau

Any reason not to do the same to the similar block of code in NodeTraceStateObserver::OnTraceEnabled()?

node/src/node.cc

Lines 239 to 258 in 59a8324

    
           #if HAVE_OPENSSL 
        
               // Stupid code to slice out the version string. 
        
               {  // NOLINT(whitespace/braces) 
        
                 size_t i, j, k; 
        
                 int c; 
        
                 for (i = j = 0, k = sizeof(OPENSSL_VERSION_TEXT) - 1; i < k; ++i) { 
        
                   c = OPENSSL_VERSION_TEXT[i]; 
        
                   if ('0' <= c && c <= '9') { 
        
                     for (j = i + 1; j < k; ++j) { 
        
                       c = OPENSSL_VERSION_TEXT[j]; 
        
                       if (c == ' ') 
        
                         break; 
        
                     } 
        
                     break; 
        
                   } 
        
                 } 
        
                 trace_process->SetString("openssl", 
        
                                         std::string(&OPENSSL_VERSION_TEXT[i], j - i)); 
        
               } 
        
           #endif

?

richardlau · 2018-09-24T11:58:40Z

src/node.cc

-      }
-    }
+  {
+    // sample opnssl version string format


thanks @richardlau - made both the changes. Didn't know there were same patterns elsewhere.

I did point it out in #22712 (comment) but given the number of comments in that PR it's easily overlooked/forgotten.

refack · 2018-09-24T14:10:39Z

src/node.cc

+    {
+      // sample openssl version string format
+      // for reference: "OpenSSL 1.1.0i  14 Aug 2018"
+      std::string ssl(OPENSSL_VERSION_TEXT);


Could you wrap the first 5 lines into a constexpr string()(const char*) function, and reuse in the second place?

@refack - sure, given OPENSSL_VERSION_TEXT is accessible everywhere (in the caller and the callee) do you mind if I don't pass it? are there any guidance on that? thanks.

Yes, of course, but I gave it a try, and we might need C++17 for this to work as expected.
But is could definatly be inline

refack · 2018-09-24T14:20:31Z

src/node.cc

+      size_t first = ssl.find(" ");
+      size_t second = ssl.find(" ", first + 1);
+      CHECK_GT(second, first);
+      ssl = ssl.substr(first + 1, second - first - 1);


Please declare a new variable.

sure, but why?

You are reusing a variable for no good reason. substr will do a copy anyway.

gireeshpunathil · 2018-09-24T15:33:32Z

@refack - pushed changes as you suggested, please have a look. thanks.

refack · 2018-09-24T16:01:51Z

src/node.h

@@ -487,7 +487,7 @@ NODE_DEPRECATED("Use WinapiErrnoException(isolate, ...)",
 #endif

 const char* signo_string(int errorno);
-
+const std::string GetOpenSSLVersion();


I'd rather not declare this as a public API.
IMHO no declaration is required, just put in higher in node.cc, but if you feel a decl is required, ~~I think util.h~~ I agree node_crypto.h is a better place.

If this is split out into a separate function, it should be defined in node_crypto.h, as is all other OpenSSL-specific code.

It could be useful in the node-report PR, especially if we go down the JSON format route (where I think this sliced version string is more appropriate than the full string currently used). But declaring the function (in node_crypto.h) could easily be done over in that PR.

@richardlau Regarding the node-report PR … it still wouldn’t have to be public API, right?

Regarding the node-report PR … it still wouldn’t have to be public API, right?

Correct. It doesn't need to be declared at all in this PR (as it's only used in src/node.cc). For use in the node-report PR it should be declared as a node internal.

followed the suggestions; moved things to node-crypto.

to be clear: what is a public API in this context? symbols accessible outside of node.exe? methods visible and callable when node is a shared library? AFAIK visibility of symbols in object files are platform dependent, and are managed (for embedders) manually at the moment? In the absence of that, every method is visible in say Linux? or is it that we follow a convention that all declared methods in node.h is deemed public? please let me know. thanks!

what is a public API in this context

Methods in headers that are included in the header tarball and not guarded by NODE_WANT_INTERNALS, i.e.:

#include <node.h> #include <node_api.h> #include <node_buffer.h> #include <node_version.h> #include <node_object_wrap.h>

addaleax · 2018-09-24T16:40:01Z

src/node.cc

+  size_t second = ssl.find(" ", first + 1);
+  CHECK_GT(second, first);
+  std::string result = ssl.substr(first + 1, second - first - 1);
+  return result;


I think you can just return ssl.substr(…);?

👍 since in C++14 it's not guaranteed to elide

thefourtheye · 2018-09-25T05:11:54Z

src/node_crypto.cc

+  size_t first = ssl.find(" ");
+  size_t second = ssl.find(" ", first + 1);
+  CHECK_GT(second, first);
+  std::string result = ssl.substr(first + 1, second - first - 1);


Nit: Can we simply return the result of substr?

@thefourtheye - please see
#23050 (comment) @refack 's response to the same: looks like that may break in c++14 world. I don't know it myself, so following SME opinion. Hope you are good with that?

It looks like @refack is actually okay with that suggestion, can you please confirm @refack? According to https://en.wikipedia.org/wiki/Copy_elision#Summary,

an implementation may omit a copy operation resulting from a return statement, even if the copy constructor has side effects.

So, there is no guarantee that a copy will be made and we should still be fine here.

@gireeshpunathil @thefourtheye I think @refack’s comment was agreeing that this might otherwise not elide if we don’t return the result directly. And it’s unavoidable to create a new string object here, since substr() already does that.

But either way: This is not performance-critical code. Just go with the simplest form. :)

thanks, reduced to the simplest form.

Ohh yes please. Copy elision is a good thing that we may get for free in C++17, but for now we need to be more explicit.

addaleax · 2018-09-25T09:06:23Z

src/node_crypto.h

@@ -98,6 +98,7 @@ extern int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx);
 extern void UseExtraCaCerts(const std::string& file);

 void InitCryptoOnce();
+const std::string GetOpenSSLVersion();


Btw, const on return types is ignored and at least some versions of GCC provide compiler warnings for that… might be best to just omit it.

thanks, removed it.

richardlau · 2018-09-25T12:16:15Z

Commit message has a typo: pacth->patch.

The openssl version as defined in ssl libraries is complex. The current logic to extract the major.minor.patch format uses C semantics to loop through the text and search for specific patterns. Use C++ string to tidy it up.

gireeshpunathil · 2018-09-25T12:30:47Z

@richardlau - thanks, fixed.

refack · 2018-09-25T14:50:11Z

@gireeshpunathil thank you for following up on all the twists and turns 💟

bnoordhuis · 2018-09-26T20:36:01Z

A different approach would be to compute it from OPENSSL_VERSION_NUMBER:

#ifdef OPENSSL_FIPS
  static const char fips[] = "-fips";
#else
  static const char fips[] = "";
#endif
  char s[32];
  uint8_t major = OPENSSL_VERSION_NUMBER >> 28;
  uint8_t minor = OPENSSL_VERSION_NUMBER >> 20;
  uint8_t fix = OPENSSL_VERSION_NUMBER >> 12;
  uint8_t patch = (OPENSSL_VERSION_NUMBER & 240) - 39;  // 'a', 'b', etc.
  snprintf(s, sizeof(s), "%d.%d.%d%c%s", major, minor, fix, patch, fips);

I'm reasonably sure most compilers will be able to optimize that to a constant expression.

gireeshpunathil · 2018-09-27T09:45:26Z

@bnoordhuis - thanks for the suggestion

   0x4005cd <main>:	push   rbp
   0x4005ce <main+1>:	mov    rbp,rsp
=> 0x4005d1 <main+4>:	sub    rsp,0x40
   0x4005d5 <main+8>:	mov    BYTE PTR [rbp-0x1],0x1  // '1'
   0x4005d9 <main+12>:	mov    BYTE PTR [rbp-0x2],0x1  // '1'
   0x4005dd <main+16>:	mov    BYTE PTR [rbp-0x3],0x0  // '0'
   0x4005e1 <main+20>:	mov    BYTE PTR [rbp-0x4],0x69 // 'i'
   0x4005e5 <main+24>:	movsx  ecx,BYTE PTR [rbp-0x4]
   0x4005e9 <main+28>:	movsx  edi,BYTE PTR [rbp-0x3]
   0x4005ed <main+32>:	movsx  esi,BYTE PTR [rbp-0x2]
   0x4005f1 <main+36>:	movsx  edx,BYTE PTR [rbp-0x1]
   0x4005f5 <main+40>:	lea    rax,[rbp-0x30]
   0x4005f9 <main+44>:	mov    QWORD PTR [rsp+0x8],0x4006f1
   0x400602 <main+53>:	mov    DWORD PTR [rsp],ecx
   0x400605 <main+56>:	mov    r9d,edi
   0x400608 <main+59>:	mov    r8d,esi
   0x40060b <main+62>:	mov    ecx,edx
   0x40060d <main+64>:	mov    edx,0x4006e0
   0x400612 <main+69>:	mov    esi,0x20
   0x400617 <main+74>:	mov    rdi,rax
   0x40061a <main+77>:	mov    eax,0x0
   0x40061f <main+82>:	call   0x4004a0 <snprintf@plt>

the token values are pre-computed by the compiler! so the runtime effort is only for formatting.

C++ dump shows expensive calls, not comparable.

So now it is a question on performance vs. readability. Do you insist on high-performant code here? I think this sequence will be invoked exactly twice in the lifecycle of the process.

Please let me know.

mhdawson

LGTM

refack · 2018-09-27T16:05:30Z

C++ dump shows expensive calls, not comparable.

I see similar outputs from gcc and clang 🤷‍♂️ https://godbolt.org/z/dKLS8v

bnoordhuis · 2018-09-27T19:30:47Z

@gireeshpunathil Interesting. At one time gcc was able to replace constant snprintf() calls (really __builtin_snprintf()) with the preformatted string but seems it no longer does.

Here is an alternative take that does result in a constant expression at -Os and above:

#include <stdio.h>
#define OPENSSL_VERSION_TEXT "OpenSSL 1.1.0i  14 Aug 2018"

constexpr int search(const char* s, int n, int c) {
  return *s == c ? n : search(s + 1, n + 1, c);
}

int main() {
  const int start = search(OPENSSL_VERSION_TEXT, 0, ' ') + 1;
  const int end = search(OPENSSL_VERSION_TEXT, start, ' ') - 1;
  const int len = end - start;
  printf("%.*s\n", len, &OPENSSL_VERSION_TEXT[start]);
}

Output:

00000000004004f8 <main>:
  4004f8:       50                      push   %rax
  4004f9:       bf c0 05 40 00          mov    $0x4005c0,%edi
  4004fe:       be 06 00 00 00          mov    $0x6,%esi
  400503:       ba ac 05 40 00          mov    $0x4005ac,%edx
  400508:       31 c0                   xor    %eax,%eax
  40050a:       e8 f1 fe ff ff          callq  400400 <printf@plt>
  40050f:       31 c0                   xor    %eax,%eax
  400511:       59                      pop    %rcx
  400512:       c3                      retq

I think this sequence will be invoked exactly twice in the lifecycle of the process.

That's true, but it happens at start-up. On slow systems (think Raspberry Pis and such) Node's start-up times are not great so we should try within reason to shave off cycles where we can.

gireeshpunathil · 2018-09-28T08:07:34Z

thaanks @bnoordhuis, pushed a new change based on your suggestion, and added you as a co-author. Few points:

std::string do not work with constexpr
your code had to be modified thus to make it work for all scenarios:

- const int end = search(OPENSSL_VERSION_TEXT, start, ' ') - 1;
+ const int end = search(OPENSSL_VERSION_TEXT + start, start, ' ') + 1;

if I replace repeated usage of OPENSSL_VERSION_TEXT with const char *str = OPENSSL_VERSION_TEXT then search is not evaluated at compile time, instead a call is made; don't know it is an issue with my compiler or not (gcc 4.8.5 20150623)

bnoordhuis · 2018-09-29T23:46:56Z

src/node_crypto.cc

+  // for reference: "OpenSSL 1.1.0i 14 Aug 2018"
+  char buf[128];
+  const int start = search(OPENSSL_VERSION_TEXT, 0, ' ') + 1;
+  const int end = search(OPENSSL_VERSION_TEXT + start, start, ' ') + 1;


Sorry, I posted a mildly incorrect version of search() (even though it looks like it worked out okay; amazing, computers usually aren't that forgiving.)

It should look like this:

constexpr int search(const char* s, int n, int c) { return s[n] == c ? n : search(s, n + 1, c); // note: s isn't incremented }

And the search for end should therefore be this:

const int end = search(OPENSSL_VERSION_TEXT, start, ' ') - 1; // note: -1 to exclude the blank

I guess you meant:
const int end = search(ssl, start, ' ') + 1;
instead of:
const int end = search(ssl, start, ' ') - 1; ?

else I get truncated output: for example when expecting 1.1.0i-fips I get 1.1.0i-fi

How about this:

constexpr int SSLVersion_Helper2(const int s) { return OPENSSL_VERSION_TEXT[s + 7] == ' ' ? s + 7 : s + 12; } constexpr const int SSLVersion_Helper() { // option 1 "OpenSSL 1.1.0i 14 Aug 2018" // option 2 "OpenSSL 1.1.0i-fips 14 Aug 2018" // 7 = strlen("OpenSSL"); static_assert(OPENSSL_VERSION_TEXT[7] == ' ', OPENSSL_VERSION_TEXT " + 7 isn't a ' ' char"); // 7 = strlen("OpenSSL 1.1.0i"); // 12 = strlen("OpenSSL 1.1.0i-fips"); static_assert(OPENSSL_VERSION_TEXT[SSLVersion_Helper2(7)] == ' ', OPENSSL_VERSION_TEXT " + 14 or 19 isn't a ' ' char"); return SSLVersion_Helper2(7); } const std::string kSSLVersionString{OPENSSL_VERSION_TEXT + 7, OPENSSL_VERSION_TEXT + SSLVersion_Helper()};

As K.I.S.S. as I could while it still compiles on GCC4.9

thanks @refack . I am seeing it as compiled into a regular function, with no static awareness of the const string or const int passed to it and leveraged by the compiler. Is it only my gcc? can you please check?

(gdb) x/16i SSLVersion_Helper2 0x400eb8 <_Z18SSLVersion_Helper2i>: push rbp 0x400eb9 <_Z18SSLVersion_Helper2i+1>: mov rbp,rsp 0x400ebc <_Z18SSLVersion_Helper2i+4>: mov DWORD PTR [rbp-0x4],edi 0x400ebf <_Z18SSLVersion_Helper2i+7>: mov eax,DWORD PTR [rbp-0x4] 0x400ec2 <_Z18SSLVersion_Helper2i+10>: add eax,0x7 0x400ec5 <_Z18SSLVersion_Helper2i+13>: cdqe 0x400ec7 <_Z18SSLVersion_Helper2i+15>: movzx eax,BYTE PTR [rax+0x401228] 0x400ece <_Z18SSLVersion_Helper2i+22>: cmp al,0x20 0x400ed0 <_Z18SSLVersion_Helper2i+24>: jne 0x400eda <_Z18SSLVersion_Helper2i+34> 0x400ed2 <_Z18SSLVersion_Helper2i+26>: mov eax,DWORD PTR [rbp-0x4] 0x400ed5 <_Z18SSLVersion_Helper2i+29>: add eax,0x7 0x400ed8 <_Z18SSLVersion_Helper2i+32>: jmp 0x400ee0 <_Z18SSLVersion_Helper2i+40> 0x400eda <_Z18SSLVersion_Helper2i+34>: mov eax,DWORD PTR [rbp-0x4] 0x400edd <_Z18SSLVersion_Helper2i+37>: add eax,0xc 0x400ee0 <_Z18SSLVersion_Helper2i+40>: pop rbp 0x400ee1 <_Z18SSLVersion_Helper2i+41>: ret (gdb)

in other words, where do we see the benefit of the constexpr optimizations that was the theme of this PR for sometime.

more for the academics: https://godbolt.org/z/qWyDbk show it being compile time only (with -O2)

addaleax

Still LGTM

addaleax · 2018-10-10T03:17:55Z

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

addaleax · 2018-10-12T04:28:16Z

Landed in d3d6cd3

The openssl version as defined in ssl libraries is complex. The current logic to extract the major.minor.patch format uses C semantics to loop through the text and search for specific patterns. Use C++ string to tidy it up. PR-URL: #23050 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

bnoordhuis · 2018-10-12T20:24:25Z

src/node_crypto.cc

+  const int start = search(OPENSSL_VERSION_TEXT, 0, ' ') + 1;
+  const int end = search(OPENSSL_VERSION_TEXT + start, start, ' ') + 1;
+  const int len = end - start;
+  snprintf(buf, len, "%.*s\n", len, &OPENSSL_VERSION_TEXT[start]);


The second arg should have been sizeof(buf), not len. I'll open a pull request. (edit: #23622)

The openssl version as defined in ssl libraries is complex. The current logic to extract the major.minor.patch format uses C semantics to loop through the text and search for specific patterns. Use C++ string to tidy it up. PR-URL: #23050 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 24, 2018

addaleax approved these changes Sep 24, 2018

View reviewed changes

src/node.cc Outdated

}

}

{

// sample opnssl version stirng format

Copy link

Member

addaleax Sep 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: string

gireeshpunathil force-pushed the ssl_version branch 2 times, most recently from 4bc66c8 to aee216b Compare September 24, 2018 09:38

gireeshpunathil changed the title ~~src: improve SLL version extraction logic~~ src: improve SSL version extraction logic Sep 24, 2018

tniessen approved these changes Sep 24, 2018

View reviewed changes

cjihrig approved these changes Sep 24, 2018

View reviewed changes

richardlau reviewed Sep 24, 2018

View reviewed changes

richardlau approved these changes Sep 24, 2018

View reviewed changes

refack suggested changes Sep 24, 2018

View reviewed changes

refack added the openssl Issues and PRs related to the OpenSSL dependency. label Sep 24, 2018

refack reviewed Sep 24, 2018

View reviewed changes

gireeshpunathil force-pushed the ssl_version branch from c47e24a to 92e96bc Compare September 24, 2018 15:31

refack reviewed Sep 24, 2018

View reviewed changes

addaleax reviewed Sep 24, 2018

View reviewed changes

gireeshpunathil force-pushed the ssl_version branch from d704536 to 915f478 Compare September 25, 2018 04:59

thefourtheye approved these changes Sep 25, 2018

View reviewed changes

addaleax reviewed Sep 25, 2018

View reviewed changes

gireeshpunathil force-pushed the ssl_version branch from 49e42d1 to 315dda0 Compare September 25, 2018 11:07

src: improve SSL version extraction logic

aa8470e

The openssl version as defined in ssl libraries is complex. The current logic to extract the major.minor.patch format uses C semantics to loop through the text and search for specific patterns. Use C++ string to tidy it up.

gireeshpunathil force-pushed the ssl_version branch from 315dda0 to aa8470e Compare September 25, 2018 12:30

jasnell approved these changes Sep 25, 2018

View reviewed changes

refack approved these changes Sep 25, 2018

View reviewed changes

lpinca approved these changes Sep 26, 2018

View reviewed changes

mhdawson approved these changes Sep 27, 2018

View reviewed changes

fixup: use constexpr to optimize

0ad3a8d

bnoordhuis reviewed Sep 29, 2018

View reviewed changes

addaleax approved these changes Oct 1, 2018

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 10, 2018

addaleax mentioned this pull request Oct 10, 2018

node-report: meld into core #22712

Merged

4 tasks

addaleax closed this Oct 12, 2018

bnoordhuis reviewed Oct 12, 2018

View reviewed changes

refack mentioned this pull request Oct 25, 2018

test: fix test-bootstrap-modules to account for workers module #23876

Closed

4 tasks

codebytere mentioned this pull request Nov 27, 2018

v10.13.1 proposal #24675

Closed

codebytere mentioned this pull request Nov 29, 2018

v10.14.2 proposal #24727

Merged

	#if HAVE_OPENSSL
	// Stupid code to slice out the version string.
	{ // NOLINT(whitespace/braces)
	size_t i, j, k;
	int c;
	for (i = j = 0, k = sizeof(OPENSSL_VERSION_TEXT) - 1; i < k; ++i) {
	c = OPENSSL_VERSION_TEXT[i];
	if ('0' <= c && c <= '9') {
	for (j = i + 1; j < k; ++j) {
	c = OPENSSL_VERSION_TEXT[j];
	if (c == ' ')
	break;
	}
	break;
	}
	}
	trace_process->SetString("openssl",
	std::string(&OPENSSL_VERSION_TEXT[i], j - i));
	}
	#endif

src: improve SSL version extraction logic #23050

src: improve SSL version extraction logic #23050

Conversation

gireeshpunathil commented Sep 24, 2018

Checklist

nodejs-github-bot commented Sep 24, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gireeshpunathil commented Sep 24, 2018

richardlau 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

refack Sep 24, 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

gireeshpunathil commented Sep 24, 2018

refack Sep 24, 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

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

richardlau commented Sep 25, 2018

gireeshpunathil commented Sep 25, 2018

refack commented Sep 25, 2018

bnoordhuis commented Sep 26, 2018

gireeshpunathil commented Sep 27, 2018

mhdawson left a comment

Choose a reason for hiding this comment

refack commented Sep 27, 2018

bnoordhuis commented Sep 27, 2018

gireeshpunathil commented Sep 28, 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

addaleax left a comment

Choose a reason for hiding this comment

addaleax commented Oct 10, 2018

addaleax commented Oct 12, 2018

bnoordhuis Oct 12, 2018 • edited Loading

Choose a reason for hiding this comment

refack Sep 24, 2018 •

edited

Loading

refack Sep 24, 2018 •

edited

Loading

bnoordhuis Oct 12, 2018 •

edited

Loading