lib: remove unnecessary symbols #22455

ghost · 2018-08-22T04:19:13Z

Remove (...), because this is a simple,sensitive expression.

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]

Trott · 2018-08-22T04:51:20Z

lib/_stream_readable.js

@@ -325,7 +325,7 @@ Readable.prototype.setEncoding = function(enc) {
 // Don't raise the hwm > 8MB
 const MAX_HWM = 0x800000;
 function computeNewHighWaterMark(n) {
-  if (n >= MAX_HWM) {
+  if (n > MAX_HWM) {


This change causes the else branch to be taken when n is equal to MAX_HWM. That seems less efficient then the if branch. ~~and possibly a bug?~~

OK, Thanks I've removed that modification.

Remove `(...)`, because this is a simple,sensitive expression.

richardlau · 2018-08-22T10:35:45Z

This appears to be the sort of stylistic changes being discussed in #22351.

ghost · 2018-08-22T11:18:15Z

@richardlau：But you have to admit that this (……) is useless here.
And this makes your style not in union :)

Please compare with them:

node/lib/_stream_readable.js

Line 78 in 6acb550

isDuplex = stream instanceof Stream.Duplex;
node/lib/_stream_readable.js

Line 150 in 6acb550

const isDuplex = (this instanceof Stream.Duplex);

So sometimes for those useless styles or styles aren't necessary (But not change to a style that will make bad effects on what the codes were). I think it worth changing to make tiny (but not much) refactors.

Hope you can understand what I mean here.

BridgeAR · 2018-08-22T11:42:11Z

I am -0.5 on this. There might be a eslint rule for this and it would be fine for me with a rule if that does not result in to much churn.

mscdex · 2018-08-22T12:04:02Z

FWIW I generally prefer including conditional statements like this in parentheses when being assigned as a value as it makes it easier to read and that's why these parentheses currently exist there.

ghost · 2018-08-23T00:42:30Z

I think it's time to have a disscussion on when to refactor（IMO）：

First, as what I said above, refactoring codes cannot make bad effects on what the codes were (including making performance lower than before, making hard to read / understand, or making unit tests too complicated to complete……ect). Anyway, any bad effects resulted by refactoring will be abandoned. And what @richardlau's closed example just belongs to this (because using IndexOf's performance isn't better, even lower than three if's. So this refactor isn't necessary).

Second, to make codes in style union. I mostly agree with what @BridgeAR said, but I have to say that even if when we all pass the eslint's checks, sometimes the stylish still will have useless or unnecessary things to be removed (Just like this fixture above). And the reason why we will do refactoring is that we should clean up codes to make them in union and look better, and obviously our Nodejs project belongs to the developers all over the world. Since we cannot control different people's options of code style, why don't we make them in union for obviously stylish problems? :)

As for @mscdex's suggestions, this is still in discussion. If most of you think for a condition wrapped by (……) —— I mean if the style looks better (though I disagree with that), I'll still make a change according to what you said (Maybe

node/lib/_stream_readable.js

Line 78 in 6acb550

isDuplex = stream instanceof Stream.Duplex;

should be changed by wrapping it with (……)).

addaleax

I think larger patches are much more relevant when talking about “churn”. A +1/-1 change is not getting in the way of anything – if it can be backported, great, if it can’t, it doesn’t make anything worse either in terms of merge conflicts.

ghost · 2018-08-23T01:02:51Z

@addaleax：Yes I agree with you.

And I think for those rules we should refactor codes:

Code styles that makes us feel puzzled, hard to understand.
Codes with useless things or symbols.
Codes with unused parameters, varaiables,……ect.
A function with the same logic but written everywhere when needed, we should take this into a global locaion.
Code styles that makes a huge performance improved (benchmark test can prove this, but what is the defination of a huge performace? 10% or 20%? This needs discussion).

Any other better ideas in details? It's interesting to share ideas and experience with all of you here :D

addaleax · 2018-08-27T22:06:15Z

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

I’d prefer to land this in the next few days rather than having it hang in limbo, given that there are 8 approving reviews at this point.

Trott · 2018-09-01T13:58:27Z

Resume build: https://ci.nodejs.org/job/node-test-pull-request/16920/

mcollina

LGTM

Trott · 2018-09-03T21:44:02Z

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

ghost · 2018-09-11T03:15:37Z

Maybe this can be merged?

Trott · 2018-09-11T23:38:03Z

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

addaleax · 2018-09-12T10:58:45Z

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

tniessen · 2018-09-12T14:56:34Z

CI was stopped, resuming again... https://ci.nodejs.org/job/node-test-pull-request/17149/

Trott · 2018-09-13T04:28:53Z

Only red CI was arm-fanned. Re-run: https://ci.nodejs.org/job/node-test-commit-arm-fanned/3457/

addaleax · 2018-09-13T08:51:25Z

Landed in 9c6f59e 😄

Remove `(...)`, because this is a simple,sensitive expression. PR-URL: #22455 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

ghost · 2018-09-13T09:37:36Z

Thanks all for your help!

Remove `(...)`, because this is a simple,sensitive expression. PR-URL: #22455 Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Aug 22, 2018

Trott reviewed Aug 22, 2018

View reviewed changes

lib: remove unnecessary symbols

8b24d12

Remove `(...)`, because this is a simple,sensitive expression.

starkwang approved these changes Aug 22, 2018

View reviewed changes

trivikr approved these changes Aug 22, 2018

View reviewed changes

lpinca approved these changes Aug 22, 2018

View reviewed changes

cjihrig approved these changes Aug 22, 2018

View reviewed changes

addaleax approved these changes Aug 23, 2018

View reviewed changes

devsnek approved these changes Aug 23, 2018

View reviewed changes

TimothyGu approved these changes Aug 23, 2018

View reviewed changes

tniessen approved these changes Aug 23, 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 Aug 27, 2018

mcollina approved these changes Sep 3, 2018

View reviewed changes

jasnell approved these changes Sep 12, 2018

View reviewed changes

addaleax closed this Sep 13, 2018

ghost deleted the FixTypos branch September 13, 2018 09:37

targos mentioned this pull request Sep 18, 2018

Release proposal: v10.11.0 #22932

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: remove unnecessary symbols #22455

lib: remove unnecessary symbols #22455

ghost commented Aug 22, 2018 •

edited by ghost

Loading

Trott Aug 22, 2018 •

edited

Loading

ghost Aug 22, 2018

richardlau commented Aug 22, 2018

ghost commented Aug 22, 2018

BridgeAR commented Aug 22, 2018 •

edited

Loading

mscdex commented Aug 22, 2018

ghost commented Aug 23, 2018 •

edited by ghost

Loading

addaleax left a comment •

edited

Loading

ghost commented Aug 23, 2018

addaleax commented Aug 27, 2018

Trott commented Sep 1, 2018

mcollina left a comment

Trott commented Sep 3, 2018

ghost commented Sep 11, 2018

Trott commented Sep 11, 2018

addaleax commented Sep 12, 2018

tniessen commented Sep 12, 2018

Trott commented Sep 13, 2018

addaleax commented Sep 13, 2018

ghost commented Sep 13, 2018

lib: remove unnecessary symbols #22455

lib: remove unnecessary symbols #22455

Conversation

ghost commented Aug 22, 2018 • edited by ghost Loading

Trott Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

ghost Aug 22, 2018

Choose a reason for hiding this comment

richardlau commented Aug 22, 2018

ghost commented Aug 22, 2018

BridgeAR commented Aug 22, 2018 • edited Loading

mscdex commented Aug 22, 2018

ghost commented Aug 23, 2018 • edited by ghost Loading

addaleax left a comment • edited Loading

Choose a reason for hiding this comment

ghost commented Aug 23, 2018

addaleax commented Aug 27, 2018

Trott commented Sep 1, 2018

mcollina left a comment

Choose a reason for hiding this comment

Trott commented Sep 3, 2018

ghost commented Sep 11, 2018

Trott commented Sep 11, 2018

addaleax commented Sep 12, 2018

tniessen commented Sep 12, 2018

Trott commented Sep 13, 2018

addaleax commented Sep 13, 2018

ghost commented Sep 13, 2018

ghost commented Aug 22, 2018 •

edited by ghost

Loading

Trott Aug 22, 2018 •

edited

Loading

BridgeAR commented Aug 22, 2018 •

edited

Loading

ghost commented Aug 23, 2018 •

edited by ghost

Loading

addaleax left a comment •

edited

Loading