src: add incr/decr operators for Reference #19083

danbev · 2018-03-02T11:44:45Z

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

danbev · 2018-03-02T11:45:23Z

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

danbev · 2018-03-02T13:45:02Z

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)
  ...

richardlau · 2018-03-02T17:32:04Z

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.

TimothyGu · 2018-03-02T20:48:23Z

src/aliased_buffer.h

+    template <typename T>
+    inline Reference& operator-=(const T& val) {
+      const T current = aliased_buffer_->GetValue(index_);
+      aliased_buffer_->SetValue(index_, current - val);


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

addaleax

Awesome, thank you!

addaleax · 2018-03-02T21:40:55Z

src/aliased_buffer.h

+    }
+
+    inline Reference& operator-=(const Reference& val) {
+      this->operator-=(static_cast<NativeT>(val));


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!

danbev · 2018-03-05T13:38:24Z

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

addaleax

This is definitely still merge-able :)

TimothyGu · 2018-03-08T16:58:29Z

src/aliased_buffer.h

+
+    template <typename T>
+    inline Reference& operator-=(const T& val) {
+      return this->operator+=(-val);


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.

danbev · 2018-03-09T06:57:10Z

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-03-09T16:07:42Z

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

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 <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Tiancheng "Timothy" Gu <[email protected]>

jasnell · 2018-08-17T23:30:11Z

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 pending requests for changes, 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: add incr/decr operators for Reference #19083

src: add incr/decr operators for Reference #19083

danbev commented Mar 2, 2018

danbev commented Mar 2, 2018

danbev commented Mar 2, 2018

richardlau commented Mar 2, 2018 •

edited

Loading

TimothyGu Mar 2, 2018

danbev Mar 3, 2018

danbev Mar 5, 2018

danbev Mar 8, 2018

addaleax left a comment

addaleax Mar 2, 2018

danbev Mar 3, 2018

danbev commented Mar 5, 2018

addaleax left a comment

TimothyGu Mar 8, 2018

TimothyGu Mar 8, 2018

danbev commented Mar 9, 2018

jasnell commented Mar 9, 2018

jasnell commented Aug 17, 2018

src: add incr/decr operators for Reference #19083

src: add incr/decr operators for Reference #19083

Conversation

danbev commented Mar 2, 2018

Checklist

Affected core subsystem(s)

danbev commented Mar 2, 2018

danbev commented Mar 2, 2018

richardlau commented Mar 2, 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

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Mar 5, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Mar 9, 2018

jasnell commented Mar 9, 2018

jasnell commented Aug 17, 2018

richardlau commented Mar 2, 2018 •

edited

Loading