src: add incr/decr operators for Reference by danbev · Pull Request #19083 · nodejs/node · GitHub

This commit adds operator overloads for increment/decrement to
AliasedBuffer::Reference.
The motivation for doing this is to hopefully make code that needs
to increment/decrement a little simpler.

Checklist

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

Affected core subsystem(s)

src

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

node-test-commit-windows-fanned failure looks unrelated

console output:

not ok 230 parallel/test-http2-https-fallback
  ---
  duration_ms: 0.264
  severity: fail
  stack: |-
    (node:3544) ExperimentalWarning: The http2 module is an experimental API.
    assert.js:243
        throw err;
        ^
    
    AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
    
      assert.ok(/Unknown ALPN Protocol, expected `h2` to be available/.test(text))
    
        at TLSSocket.tls.setEncoding.on.on.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-http2-https-fallback.js:143:9)
        at TLSSocket.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:467:15)
        at TLSSocket.emit (events.js:136:15)
        at endReadableNT (_stream_readable.js:1021:12)
        at process._tickCallback (internal/process/next_tick.js:115:19)
  ...

node-test-commit-windows-fanned failure looks unrelated

      assert.ok(/Unknown ALPN Protocol, expected `h2` to be available/.test(text))
   
       at TLSSocket.tls.setEncoding.on.on.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-http2-https-fallback.js:143:9)

This assert was recently introduced in #18986
cc @addaleax

Update: being addressed by #19093.

Unless I’m missing something, you could just do operator+=(index_, -val) here. If you add an overload for the unary minus operator (or is that even necessary?), that should get rid of the overload below that takes in a const Reference&.

I'll take a closer look at your suggestion on Monday (little short of time right now). Thanks

@TimothyGu Please take a look and see if this was what you had in mind, thanks.

@TimothyGu If possible I'd like get this merged but it would be great if you could take a look at the latest change and see if this is what you had in mind. Thanks

Awesome, thank you!

Just a suggestion, but I would personally find return *this -= static_cast<NativeT>(val); a bit clearer/more idiomatic in these places.

Ah yes, let me change that thanks!

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

This is definitely still merge-able :)

Just making sure, this still works if val is a reference?

Would definitely be good to test for that.

This commit adds operator overloads for increment/decrement to AliasedBuffer::Reference. The motivation for doing this is to hopefully make code that needs to increment/decrement a little simpler.

Test added and rebased CI: https://ci.nodejs.org/job/node-test-pull-request/13599/

This commit adds operator overloads for increment/decrement to AliasedBuffer::Reference. The motivation for doing this is to hopefully make code that needs to increment/decrement a little simpler. PR-URL: #19083 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Landed in 45277e2

This commit adds operator overloads for increment/decrement to AliasedBuffer::Reference. The motivation for doing this is to hopefully make code that needs to increment/decrement a little simpler. PR-URL: #19083 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

This commit adds operator overloads for increment/decrement to AliasedBuffer::Reference. The motivation for doing this is to hopefully make code that needs to increment/decrement a little simpler. PR-URL: nodejs#19083 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

Should this be backported to 8.x? If so, a separate backport PR is necessary.

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

jasnell approved these changes Mar 2, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 2, 2018

TimothyGu reviewed Mar 2, 2018

View reviewed changes

addaleax approved these changes Mar 2, 2018

View reviewed changes

tniessen approved these changes Mar 4, 2018

View reviewed changes

cjihrig approved these changes Mar 5, 2018

View reviewed changes

addaleax approved these changes Mar 8, 2018

View reviewed changes

TimothyGu approved these changes Mar 8, 2018

View reviewed changes

danbev added 5 commits March 9, 2018 04:41

src: add incr/decr operators for Reference

5cb22f6

This commit adds operator overloads for increment/decrement to AliasedBuffer::Reference. The motivation for doing this is to hopefully make code that needs to increment/decrement a little simpler.

squash: fix return statements

4fca5f7

squash: call inc operator for decr operator

5c687c0

squash: add test for overload operators

651bfb2

squash: add test for Reference overloads

18f46e8

danbev force-pushed the aliased_buffer_plus_minus_equals_overload branch from 6802731 to 18f46e8 Compare March 9, 2018 06:56

TimothyGu approved these changes Mar 9, 2018

View reviewed changes

jasnell closed this Mar 9, 2018

danbev deleted the aliased_buffer_plus_minus_equals_overload branch March 12, 2018 05:56

targos mentioned this pull request Mar 18, 2018

v9.9.0 proposal #19428

Merged

jasnell added the backport-requested-v8.x label Aug 17, 2018

src: add incr/decr operators for Reference by danbev · Pull Request #19083 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: add incr/decr operators for Reference - #19083

src: add incr/decr operators for Reference#19083danbev wants to merge 5 commits intonodejs:masternodejs/node:masterfrom danbev:aliased_buffer_plus_minus_equals_overloaddanbev/node:aliased_buffer_plus_minus_equals_overloadCopy head branch name to clipboard

Conversation

danbev commented Mar 2, 2018

Checklist

Affected core subsystem(s)

Uh oh!

danbev commented Mar 2, 2018

Uh oh!

danbev commented Mar 2, 2018

Uh oh!

richardlau commented Mar 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 5, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danbev commented Mar 9, 2018

Uh oh!

jasnell commented Mar 9, 2018

Uh oh!

jasnell commented Aug 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

src: add incr/decr operators for Reference#19083
danbev wants to merge 5 commits into
nodejs:masterfrom
danbev:aliased_buffer_plus_minus_equals_overload

richardlau commented Mar 2, 2018 •
edited
Loading