src: refactor out some warnings from `FillStatsArray` and `node_file.h` #23793

refack · 2018-10-21T00:46:37Z

~~#### adds a the Guideline Support Library to /deps/:~~

src: refactor out warnings from FillStatsArray

move FillStatsArray to node_file.h minimize number of template instantions
change inline to constexpr
change MACRO consts to constexpr
correct polymorphic conversion from uv_timespec_t
DRY FillGlobalStatsArray
change FillGlobalStatsArray signature to have one less default arg
reduce some includes

src: clean clang-tidy errors in node_file.h

fix bug with non virtual dtor
delete move constructors
default initialize all members
discard unneeded return values
const some possibles

Checklist

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

nodejs-github-bot · 2018-10-21T00:46:39Z

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

refack · 2018-10-21T00:47:48Z

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

refack · 2018-10-21T00:51:42Z

@addaleax, I believe MallocedBuffer<T> can be our implementation of dyn_array<T>. It seems like that one is an elusive to implement genetically, and it's missing from the current implementation of the GSL.

addaleax · 2018-10-21T01:54:02Z

This seems like a huge change for something that could be achieved through adding a couple of static_casts…

refack · 2018-10-21T01:58:21Z

This seems like a huge change for something that could be achieved through adding a couple of static_casts…

It's a refactor that makes sense. Besides eliminating the warnings, we take out a template was was instantiated twice in every translation unit it was included, while it's actually used in to. Plus reducing node_internals.h which is kinda big.

And the GSL is just a big bunch of free win.

P.S. the second commit fe6cd36 is not that big...

addaleax · 2018-10-21T02:05:10Z

I’m totally on board with moving this out of node_internals.h, sure… just, this mixes a pretty decent-size dependency in for something where we don’t really need it…

refack · 2018-10-21T02:16:19Z

I’m totally on board with moving this out of node_internals.h, sure… just, this mixes a pretty decent-size dependency in for something where we don’t really need it…

I can add it in a separate PR, I thought this would be a nice POC...

But it just found a bug on Windows:

fields->SetValue(offset + 7, gsl::narrow<NativeT>(s->st_ino))

Where NativeT == double and s->st_ino == 31525197391720945 -> (double)s->st_ino == 31525197391720944

joyeecheung

Can you use static_cast in this PR, and leave the gsl part to another one as a follow up?

Anyway I am happy to see this out of node_internals.h!👍

refack · 2018-10-21T15:23:24Z

Took out the GSL, but added a second commit with few bug fixes found with clang-tidy.
Simply instantiating FillStatsArray only twice reduces number of warning by order of magnitude (from ~900 to ~90)
PTAL.

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

addaleax · 2018-10-21T16:17:15Z

src/node_file.cc

@@ -2319,6 +2319,8 @@ void Initialize(Local<Object> target,
              use_promises_symbol).FromJust();
 }

+
+


Unrelated whitespace change

addaleax · 2018-10-21T16:17:19Z

src/node_file.h

@@ -30,7 +30,7 @@ class FSContinuationData : public MemoryRetainer {

  uv_fs_t* req;
  int mode;
-  std::vector<std::string> paths;
+  std::vector<std::string> paths{};


"ctor does not initialize paths"

That seems like something we should address on the compiler side, rather than starting to fix up all of our code… it’s not like something bad is happening here

The compiler can't tell if we want the default constructor or we forgot to initialize.
I'd rather be explicit, either here or in all the constructors.

addaleax · 2018-10-21T16:19:10Z

src/node_file.h

@@ -187,11 +263,12 @@ class FSReqPromise : public FSReqBase {
        object()->Get(env()->context(),
                      env()->promise_string()).ToLocalChecked();
    Local<Promise::Resolver> resolver = val.As<Promise::Resolver>();
-    resolver->Resolve(env()->context(), value).FromJust();
+    static_cast<void>(resolver->Resolve(env()->context(), value).FromJust());


We use USE(…) to serve this purpose (inspired by the same function in V8) rather than casting to void

IMHO it is clearer...

It's not a macro

Reffered to explicitly in (4) at https://en.cppreference.com/w/cpp/language/static_cast

Googling of static_cast<void>

https://stackoverflow.com/questions/49273384/what-is-purpose-of-using-static-castvoid

USE isn’t a macro either, fwiw, just named that way because it used to be one in V8 I think.

addaleax · 2018-10-21T16:20:21Z

src/node_file.h

 private:
  bool finished_ = false;
  AliasedBuffer<NativeT, V8T> stats_field_array_;
-  DISALLOW_COPY_AND_ASSIGN(FSReqPromise);


Can you explain these changes? If we are getting warnings here, we might want to revert 97f1e94 instead?

It's a MACRO

It in private

Does not delete the moves

I think it's fine to use a macro if it reduces repetition, and it's also easier to search..about deleting move constructor&operator, why not just add another macro? (oops, I know)

The move variants are implicitly deleted, according to #23092

In this case I'm in favor being explicit over less-code-via-macros.

As for the move semantics, they are not generated, but are left undefined so it's a clang-tidy error, since the compiler can't say what we ment (not implement or forgot to implement).

#23092 is sort of Ok, since the MACRO is named DISALLOW_COPY_AND_ASSIGN not DISALLOW_COPY_AND_ASSIGN_AND_MOVE

P.S. C++20 might solve this with metaclasses

addaleax · 2018-10-21T16:21:50Z

src/node_file.h

+constexpr void FillStatsArrays(AliasedBuffer<NativeT, V8T>* fields,
+                               const uv_stat_t* s,
+                               const uv_stat_t* s2 = nullptr) {
+    if (s2) {


We try to use explicit comparisons for pointers, i.e. if (s2 != nullptr)

I thought I saw the opposite suggestions in a review this week 🤔
Anyway our styleguide has ES.87: Don’t add redundant == or != to conditions

@refack That conflicts 100 % with our current style, i.e. it’s an omission in our style guide.

I’ve opened #23805 to document this.

refack · 2018-10-21T18:55:47Z

Reverted changes to grow_async_ids_stack in favor of #23808
https://ci.nodejs.org/job/node-test-pull-request/18032/

refack · 2018-10-21T20:15:33Z

Fixed bad static_cast (those are not as simple as you'd expect);
CI: https://ci.nodejs.org/job/node-test-commit/22525/

joyeecheung · 2018-10-23T00:20:08Z

src/node_file.h

+inline Local<Value> FillGlobalStatsArray(Environment* env,
+                                         bool use_bigint,
+                                         const uv_stat_t* s,
+                                         const uv_stat_t* s2 = nullptr) {


Is there a reason the second call is encapsulated in node_file.h? It should be easier to understand if the outmost caller just call it twice? (as that's only specific to watch methods, and only the JS part for those methods will make use of the additional fields)

It made for a nice signature, also completely abstracted that we use the same AliasedArray for both. But after I refactored it, I got rid of a function, so it's also nice. PTAL.

addaleax · 2018-10-23T02:56:09Z

src/node_file.h

@@ -30,7 +30,7 @@ class FSContinuationData : public MemoryRetainer {

  uv_fs_t* req;
  int mode;
-  std::vector<std::string> paths;
+  std::vector<std::string> paths{};


That seems like something we should address on the compiler side, rather than starting to fix up all of our code… it’s not like something bad is happening here

addaleax · 2018-10-23T02:58:47Z

src/node_file.h

+                                         bool second = false) {
+  ptrdiff_t offset = second ? kFsStatsFieldsNumber : 0;
+  if (use_bigint) {
+    const auto arr = env->fs_stats_field_bigint_array();


I understand the desire to not spell out the full type here, but could you make it const auto*? That way it’s obvious that no copy operations take place here, not knowing the type of arr

I wanted to, but the compiler was being strange, I'll check again.

error C2664: 'void node::fs::FillStatsArray<uint64_t,v8::BigUint64Array>(node::AliasedBuffer<uint64_t,v8::BigUint64Array> *,const uv_stat_t *,size_t)': cannot convert argument 1 from 'const node::AliasedBuffer<uint64_t,v8::BigUint64Array> *' to 'node::AliasedBuffer<uint64_t,v8::BigUint64Array> *' [D:\code\node\node_lib.vcxproj]

It's a ~~covariance~~ const thing.
I could define a using for the two variants...

It’s a const thing – auto*, not const auto* is what I should have written.

Ack. It's a const auto * vs auto* const.
Done.

addaleax · 2018-10-23T02:58:53Z

src/node_file.h

+    FillStatsArray(arr, s, offset);
+    return arr->GetJSArray();
+  } else {
+    const auto arr = env->fs_stats_field_array();


addaleax · 2018-10-23T03:28:17Z

src/node_file.h

@@ -319,7 +411,7 @@ class FileHandle : public AsyncWrap, public StreamBase {
      ref_.Reset(env->isolate(), ref);
    }

-    ~CloseReq() {
+    virtual ~CloseReq() {


Wouldn’t override do the job here? It’s what you used in other PRs, I think.

refack · 2018-10-23T03:34:54Z

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

refack · 2018-10-23T14:31:28Z

New CI: ~~https://ci.nodejs.org/job/node-test-pull-request/18076/~~
https://ci.nodejs.org/job/node-test-pull-request/18078/

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

* explicitly delete move overloads * default initialize all members * explicitly discard unused return values * const some possibles PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

targos · 2018-11-01T20:44:11Z

Marking dont-land-on-v11.x until nodejs/build#1542 is fixed

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

* explicitly delete move overloads * default initialize all members * explicitly discard unused return values * const some possibles PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

* explicitly delete move overloads * default initialize all members * explicitly discard unused return values * const some possibles PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

* explicitly delete move overloads * default initialize all members * explicitly discard unused return values * const some possibles PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

MylesBorins · 2019-01-29T05:06:58Z

This PR seems to be creating unexpected failures on v10.x BSD

Can someone please backport?

https://ci.nodejs.org/job/node-test-commit-freebsd/23692/nodes=freebsd10-64/console

21:57:44 In file included from ../src/env.cc:6:
21:57:44 ../src/node_file.h:161:19: error: no return statement in constexpr function
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:168:18: error: no function template matches function template specialization 'ToNative'
21:57:44 constexpr double ToNative(uv_timespec_t ts) {
21:57:44                  ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:180:20: error: no function template matches function template specialization 'ToNative'
21:57:44 constexpr uint64_t ToNative(uv_timespec_t ts) {
21:57:44                    ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:215:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 10, ToNative<NativeT>(s->st_atim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:228:5: note: in instantiation of function template specialization 'node::fs::FillStatsArray<unsigned long, v8::BigUint64Array>' requested here
21:57:44     FillStatsArray(arr, s, offset);
21:57:44     ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:216:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 11, ToNative<NativeT>(s->st_mtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:217:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 12, ToNative<NativeT>(s->st_ctim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:218:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 13, ToNative<NativeT>(s->st_birthtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = unsigned long, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:215:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 10, ToNative<NativeT>(s->st_atim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:232:5: note: in instantiation of function template specialization 'node::fs::FillStatsArray<double, v8::Float64Array>' requested here
21:57:44     FillStatsArray(arr, s, offset);
21:57:44     ^
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:216:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 11, ToNative<NativeT>(s->st_mtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:217:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 12, ToNative<NativeT>(s->st_ctim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 ../src/node_file.h:218:33: error: no matching function for call to 'ToNative'
21:57:44   fields->SetValue(offset + 13, ToNative<NativeT>(s->st_birthtim));
21:57:44                                 ^~~~~~~~~~~~~~~~~
21:57:44 ../src/node_file.h:161:19: note: candidate template ignored: substitution failure [with NativeT = double, $1 = std::__1::enable_if<true, void>]
21:57:44 constexpr NativeT ToNative(uv_timespec_t ts) {
21:57:44                   ^
21:57:44 11 errors generated.
21:57:44 gmake[2]: *** [node_lib.target.mk:316: /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/out/Release/obj.target/node_lib/src/env.o] Error 1
21:57:44 rm 25dc971a40d6cb85b6b7264b2590e351c5d97ef4.intermediate c72314b5849ba99a8583636d997488ad6da171e7.intermediate f7457ada113d3cc0ab78869adac42ba732a9fff5.intermediate
21:57:44 gmake[1]: *** [Makefile:99: node] Error 2
21:57:44 gmake: *** [Makefile:475: build-ci] Error 2
21:57:45 Build step 'Execute shell' marked build as failure
21:57:45 Performing Post build task...
21:57:45 Match found for : : True
21:57:45 Logical operation result is TRUE
21:57:45 Running script  : #/bin/bash

refack requested review from addaleax, joyeecheung and gireeshpunathil October 21, 2018 00:46

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Oct 21, 2018

refack self-assigned this Oct 21, 2018

refack 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 Oct 21, 2018

refack force-pushed the GSL branch from fe6cd36 to 86852c7 Compare October 21, 2018 02:57

joyeecheung reviewed Oct 21, 2018

View reviewed changes

refack force-pushed the GSL branch from 86852c7 to ee63a3b Compare October 21, 2018 15:12

refack changed the title ~~src: refactor out warnings from FillStatsArray~~ src: refactor out some warnings from FillStatsArray and node_file.h Oct 21, 2018

refack force-pushed the GSL branch from ee63a3b to 4194693 Compare October 21, 2018 15:29

refack removed the meta Issues and PRs related to the general management of the project. label Oct 21, 2018

addaleax reviewed Oct 21, 2018

View reviewed changes

refack force-pushed the GSL branch from edc9503 to a8e1f04 Compare October 21, 2018 18:25

joyeecheung reviewed Oct 23, 2018

View reviewed changes

addaleax approved these changes Oct 23, 2018

View reviewed changes

refack merged commit 205b667 into nodejs:master Oct 24, 2018

refack deleted the GSL branch October 24, 2018 16:13

refack removed their assignment Oct 24, 2018

targos pushed a commit that referenced this pull request Oct 26, 2018

src: refactor FillStatsArray

6467fe0

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

targos pushed a commit that referenced this pull request Oct 26, 2018

src: fix resource leak in node::fs::FileHandle

17ba595

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

targos mentioned this pull request Oct 27, 2018

Release proposal: v11.1.0 #23922

Merged

targos added the dont-land-on-v11.x label Nov 1, 2018

targos removed the dont-land-on-v11.x label Nov 18, 2018

targos pushed a commit that referenced this pull request Nov 18, 2018

src: refactor FillStatsArray

98443b5

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

targos pushed a commit that referenced this pull request Nov 18, 2018

src: fix resource leak in node::fs::FileHandle

f3dbc70

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

rvagg pushed a commit that referenced this pull request Nov 28, 2018

src: refactor FillStatsArray

c0a9a83

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

rvagg pushed a commit that referenced this pull request Nov 28, 2018

src: fix resource leak in node::fs::FileHandle

260d777

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

BridgeAR mentioned this pull request Dec 5, 2018

v11.4.0 proposal #24854

Merged

4 tasks

codebytere pushed a commit that referenced this pull request Jan 13, 2019

src: refactor FillStatsArray

acf7e7d

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere pushed a commit that referenced this pull request Jan 13, 2019

src: fix resource leak in node::fs::FileHandle

538589e

PR-URL: #23793 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>

codebytere mentioned this pull request Jan 15, 2019

v10.15.1 proposal #25346

Merged

MylesBorins added the backport-requested-v10.x label Jan 29, 2019

refack mentioned this pull request Feb 11, 2019

src: apply clang-tidy rule performance-unnecessary-value-param #26042

Closed

2 tasks

		@@ -2319,6 +2319,8 @@ void Initialize(Local<Object> target,
		use_promises_symbol).FromJust();
		}

src: refactor out some warnings from FillStatsArray and node_file.h #23793

src: refactor out some warnings from FillStatsArray and node_file.h #23793

Conversation

refack commented Oct 21, 2018 • edited Loading

src: refactor out warnings from FillStatsArray

src: clean clang-tidy errors in node_file.h

Checklist

nodejs-github-bot commented Oct 21, 2018

refack commented Oct 21, 2018

refack commented Oct 21, 2018

addaleax commented Oct 21, 2018

refack commented Oct 21, 2018 • edited Loading

addaleax commented Oct 21, 2018

refack commented Oct 21, 2018

joyeecheung left a comment

Choose a reason for hiding this comment

refack commented Oct 21, 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

joyeecheung Oct 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

refack commented Oct 21, 2018

refack commented Oct 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

refack Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

refack Oct 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

refack commented Oct 23, 2018

refack commented Oct 23, 2018 • edited Loading

targos commented Nov 1, 2018

MylesBorins commented Jan 29, 2019

src: refactor out some warnings from `FillStatsArray` and `node_file.h` #23793

src: refactor out some warnings from `FillStatsArray` and `node_file.h` #23793

refack commented Oct 21, 2018 •

edited

Loading

refack commented Oct 21, 2018 •

edited

Loading

refack commented Oct 21, 2018 •

edited

Loading

joyeecheung Oct 23, 2018 •

edited

Loading

refack Oct 23, 2018 •

edited

Loading

refack Oct 23, 2018 •

edited

Loading

refack commented Oct 23, 2018 •

edited

Loading