src: make ReqWrap req_ member private #8532

danbev · 2016-09-14T13:27:30Z

Checklist

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

Affected core subsystem(s)

src

Description of change

This commit attempts to address one of the items in
#4641 which is
related to src/req-wrap.h and making the req_ member private.

danbev · 2016-09-14T13:28:33Z

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

Fishrock123 · 2016-09-14T16:56:58Z

Is this semver-major?

Trott · 2016-09-14T18:42:59Z

@bnoordhuis

danbev · 2016-09-15T08:33:49Z

Is this semver-major?

Good point, it is changing a public field and might break something so I think that qualifies. I'll label it as semver-major. Thanks

bnoordhuis · 2016-09-15T10:19:26Z

src/stream_wrap.cc

  req_wrap->Dispatched();
  return err;
 }


 void StreamWrap::AfterShutdown(uv_shutdown_t* req, int status) {
-  ShutdownWrap* req_wrap = ContainerOf(&ShutdownWrap::req_, req);
+  ShutdownWrap* req_wrap = static_cast<ShutdownWrap*>(req->data);
+  CHECK_NE(req_wrap, nullptr);


Can you add from_req() methods that use ContainerOf? You need to make req_ protected.

I've added a commit with from_req() methods. Let me know if this was what you had in mind. Thanks

bnoordhuis · 2016-09-15T10:20:02Z

src/stream_wrap.cc

@@ -360,7 +361,8 @@ int StreamWrap::DoWrite(WriteWrap* w,


 void StreamWrap::AfterWrite(uv_write_t* req, int status) {
-  WriteWrap* req_wrap = ContainerOf(&WriteWrap::req_, req);
+  WriteWrap* req_wrap = static_cast<WriteWrap*>(req->data);


danbev · 2016-09-16T10:51:38Z

src/req-wrap.h

-  T req_;
+
+ protected:
+  T req_;  // Must be last.


I'd like to add some more information about the reason for this requirement, that it be the last member. Moving the protected member to be before the private members will cause failures in tests, but I've not been able to find the reason for this yet. @bnoordhuis Are there any resources (links, docs) you could point me to, to help me understand this?

req_wrap_queue_ needs to be at a fixed offset from the start of the struct because it is used by ContainerOf to calculate the address of the embedding ReqWrap.

ContainerOf compiles down to simple, fixed pointer arithmetic. sizeof(req_) depends on the type of T so req_wrap_queue_ would no longer be at a fixed offset if it came after req_.

It's a bit of a hack in that it depends on the layout in memory but I deemed it less worse than the untyped queue that we used before.

Ah, that makes sense. Thanks for the explanation!

Yeah, that definitely sounds worth adding to the comment here.

addaleax · 2016-09-20T11:41:22Z

Is this semver-major?

Good point, it is changing a public field and might break something so I think that qualifies. I'll label it as semver-major. Thanks

I don’t think we consider anything that’s guarded by NODE_WANT_INTERNALS part of the public API?

jasnell · 2016-09-20T18:37:19Z

@addaleax ... good point but I'm wondering if it's safer to be cautious on this, just in case?

addaleax · 2016-09-20T18:45:31Z

Sure, it probably doesn’t matter anyway. :)

addaleax

LGTM

CI: https://ci.nodejs.org/job/node-test-commit/5212/

bnoordhuis

This could be semver-patch, it only touches internals.

jasnell · 2016-09-21T17:23:08Z

Removing the semver-major label.

danbev · 2016-09-22T05:36:44Z

Thanks for all the reviews on this!

This is the first time I've submitted a PR that might affect a semver change, possibly a semver-patch as noted above, and I'm not sure what I need to do. I looked through the onboarding guide again but could only find information about adding a label to the issue tracker, and that more than one LGTM is required for a semver PR. Could someone advice me on any action I need to take?

addaleax · 2016-09-22T13:45:33Z

This is the first time I've submitted a PR that might affect a semver change, possibly a semver-patch as noted above

It’s a bit weird applying semver principles to changes that are not supposed to effect change of the visible behaviour, but we usually consider all commits that are not semver-major or semver-minor to be implicitly semver-patch. (That’s why there is no label for patches.)

and I'm not sure what I need to do.

Probably nothing. :)

and that more than one LGTM is required for a semver PR.

That should only be the case for semver-major changes, but even so, this PR already has two LGTMs from CTC members.

danbev · 2016-09-23T05:43:45Z

Probably nothing. :)

Ah great, thanks for clarifying.

danbev · 2016-09-23T05:50:19Z

Sorry, I deleted my branch by mistake when trying to clean up old PR branches. Reopening and will rebase this.

This commit attempts to address one of the items in nodejs#4641 which is related to src/req-wrap.h and making the req_ member private.

bnoordhuis provided details about the requirement of req_ having come after req_wrap_queue_, and I'm simply adding them as a comment to the req_ field.

danbev · 2016-09-23T06:32:22Z

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

This commit attempts to address one of the items in nodejs#4641 which is related to src/req-wrap.h and making the req_ member private. PR-URL: nodejs#8532 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

danbev · 2016-09-23T08:33:17Z

Landed in 0c64552

This commit attempts to address one of the items in #4641 which is related to src/req-wrap.h and making the req_ member private. PR-URL: #8532 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

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

danbev added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 15, 2016

bnoordhuis requested changes Sep 15, 2016

View reviewed changes

danbev commented Sep 16, 2016

View reviewed changes

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

addaleax approved these changes Sep 21, 2016

View reviewed changes

bnoordhuis approved these changes Sep 21, 2016

View reviewed changes

jasnell removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 21, 2016

danbev closed this Sep 23, 2016

danbev deleted the make_req_private branch September 23, 2016 05:45

danbev restored the make_req_private branch September 23, 2016 05:49

danbev reopened this Sep 23, 2016

danbev added 3 commits September 23, 2016 07:50

src: make ReqWrap req_ member private

3eeaa8d

This commit attempts to address one of the items in nodejs#4641 which is related to src/req-wrap.h and making the req_ member private.

src: add from_req methods

2a827bd

clarify comment about req_ having to be last

7fa9172

bnoordhuis provided details about the requirement of req_ having come after req_wrap_queue_, and I'm simply adding them as a comment to the req_ field.

danbev force-pushed the make_req_private branch from b0a7e5d to 7fa9172 Compare September 23, 2016 06:26

danbev closed this Sep 23, 2016

danbev deleted the make_req_private branch September 26, 2016 04:00

MylesBorins added the dont-land-on-v4.x label Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: make ReqWrap req_ member private #8532

src: make ReqWrap req_ member private #8532

danbev commented Sep 14, 2016

danbev commented Sep 14, 2016

Fishrock123 commented Sep 14, 2016

Trott commented Sep 14, 2016

danbev commented Sep 15, 2016

bnoordhuis Sep 15, 2016

danbev Sep 16, 2016

bnoordhuis Sep 15, 2016

danbev Sep 16, 2016

bnoordhuis Sep 16, 2016

danbev Sep 17, 2016

addaleax Sep 20, 2016

addaleax commented Sep 20, 2016

jasnell commented Sep 20, 2016

addaleax commented Sep 20, 2016

addaleax left a comment

bnoordhuis left a comment

jasnell commented Sep 21, 2016

danbev commented Sep 22, 2016

addaleax commented Sep 22, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016

src: make ReqWrap req_ member private #8532

src: make ReqWrap req_ member private #8532

Conversation

danbev commented Sep 14, 2016

Checklist

Affected core subsystem(s)

Description of change

danbev commented Sep 14, 2016

Fishrock123 commented Sep 14, 2016

Trott commented Sep 14, 2016

danbev commented Sep 15, 2016

bnoordhuis Sep 15, 2016

Choose a reason for hiding this comment

danbev Sep 16, 2016

Choose a reason for hiding this comment

bnoordhuis Sep 15, 2016

Choose a reason for hiding this comment

danbev Sep 16, 2016

Choose a reason for hiding this comment

bnoordhuis Sep 16, 2016

Choose a reason for hiding this comment

danbev Sep 17, 2016

Choose a reason for hiding this comment

addaleax Sep 20, 2016

Choose a reason for hiding this comment

addaleax commented Sep 20, 2016

jasnell commented Sep 20, 2016

addaleax commented Sep 20, 2016

addaleax left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

jasnell commented Sep 21, 2016

danbev commented Sep 22, 2016

addaleax commented Sep 22, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016

danbev commented Sep 23, 2016