Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: fix ARRAY_SIZE() logic error #5969

Merged
merged 3 commits into from
Apr 5, 2016
Merged

Conversation

bnoordhuis
Copy link
Member

Pull Request check-list

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to [CONTRIBUTING.md][0]?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?
  • Is a documentation update included (if this change modifies
    existing APIs, or introduces new ones)?

Affected core subsystem(s)

src

Description of change

  • src: fix ARRAY_SIZE() logic error
  • src: replace ARRAY_SIZE with typesafe arraysize
  • src: use size_t for http parser array size fields

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

@bnoordhuis
Copy link
Member Author

/cc @Fishrock123

@@ -248,7 +248,7 @@ void InitLTTNG(Environment* env, Local<Object> target) {
#undef NODE_PROBE
};

for (unsigned int i = 0; i < ARRAY_SIZE(tab); i++) {
for (unsigned int i = 0; i < arraysize(tab); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small question: Does it make sense to switch from unsigned to size_t, too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought I caught them all (I updated them in other files) but I must've overlooked this file. I'll fix that before landing.

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 31, 2016
@cjihrig
Copy link
Contributor

cjihrig commented Mar 31, 2016

LGTM

@Fishrock123
Copy link
Contributor

@bnoordhuis What issue did this cause?

seems fine if it works, I guess? I really don't understand how it works lol

@Fishrock123 Fishrock123 added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 31, 2016
@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

@bnoordhuis
Copy link
Member Author

Let's see if VS takes the hint when it's marked constexpr: https://ci.nodejs.org/job/node-test-pull-request/2122/

@bnoordhuis
Copy link
Member Author

What issue did this cause? seems fine if it works, I guess?

No issue but that's because it works by accident (because sizeof(Local<Value>) == sizeof(void*).)

@bnoordhuis
Copy link
Member Author

Once more with VS 2013 workaround... https://ci.nodejs.org/job/node-test-pull-request/2124/

@bnoordhuis
Copy link
Member Author

CI is green except for a post-test service outage on one of the ARM buildbots. I'll land this tomorrow.

@jasnell
Copy link
Member

jasnell commented Apr 1, 2016

LGTM

Bug introduced in commit 21d66d6 ("lib: remove bootstrap global context
indirection").

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis bnoordhuis closed this Apr 5, 2016
@bnoordhuis bnoordhuis deleted the arraysize branch April 5, 2016 09:38
@bnoordhuis bnoordhuis merged commit ea63f79 into nodejs:master Apr 5, 2016
@MylesBorins
Copy link
Contributor

@bnoordhuis these changes are not landing cleanly on v5.x. Would you be able to backport or handle landing it?

bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Apr 14, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Apr 14, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
Copy link
Member Author

Sorry for the delay. Back-port: #6199

@MylesBorins
Copy link
Contributor

@bnoordhuis it looks like this isn't landing cleanly on v4 (the v5 back port isn't either). We'll need to do a manual backport when the time comes)

MylesBorins pushed a commit that referenced this pull request Apr 14, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 14, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@bnoordhuis
Copy link
Member Author

@thealphanerd #6221

This was referenced Apr 20, 2016
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 2, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
bnoordhuis added a commit to bnoordhuis/io.js that referenced this pull request Jun 2, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: nodejs#5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 6, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 6, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jun 24, 2016
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
To prevent `ARRAY_SIZE(&arg)` (i.e., taking the array size of a pointer)
from happening again.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jun 24, 2016
Make the `num_values_` and `num_fields_` unsigned and remove an
erroneous comment.

PR-URL: #5969
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants