Skip to content

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 1, 2019

This just makes the code a bit more obvious (subjectively).

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

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 1, 2019
src/util.h Outdated
Copy link
Contributor

@refack refack Feb 1, 2019

Choose a reason for hiding this comment

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

If you're here, would you consider de-crufting this? i.e.:

    if (!(expr)) {                                                             \
      node::Assert({ __FILE__, __LINE__, #expr, PRETTY_FUNCTION_NAME });       \
    }                                                                          \

Copy link
Member Author

Choose a reason for hiding this comment

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

@refack At least locally (gcc-7 on Ubuntu 18.04), that would undo the purpose of this (what the comment is referring to): It would put the object construction in-line into the code instead of ending up as a simple pointer load from the read-only data section.

Copy link
Contributor

Choose a reason for hiding this comment

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

So how about:

    if (!(expr)) {                                                             \
      static constexpr args{ __FILE__, __LINE__, #expr, PRETTY_FUNCTION_NAME } \
      node::Assert(args);                                                      \
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

@refack You are talking about constconstexpr, right? That has the same result for me, and I don’t see it as being clearer about the intention here (having an explicit, hardcoded object in the read-only data section).

I’ll make the comment more explicit about what the idea here is.

@addaleax
Copy link
Member Author

addaleax commented Feb 1, 2019

@addaleax
Copy link
Member Author

addaleax commented Feb 2, 2019

@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 2, 2019
@addaleax
Copy link
Member Author

addaleax commented Feb 3, 2019

Landed in 30545a5

@addaleax addaleax closed this Feb 3, 2019
@addaleax addaleax deleted the check-args branch February 3, 2019 19:37
addaleax added a commit that referenced this pull request Feb 3, 2019
This just makes the code a bit more obvious (subjectively).

PR-URL: #25869
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
addaleax added a commit that referenced this pull request Feb 3, 2019
This just makes the code a bit more obvious (subjectively).

PR-URL: #25869
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targos targos mentioned this pull request Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants