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: move error handling code into node_errors.cc #24058

Closed
wants to merge 2 commits into from

Conversation

joyeecheung
Copy link
Member

Move the following code into a new node_errors.cc file and
declare them in node_errors.h for clarity and make it possible
to include them with node_errors.h.

  • AppendExceptionLine()
  • DecorateErrorStack()
  • FatalError()
  • OnFatalError()
  • PrintErrorString()
  • FatalException()
  • ReportException()
  • FatalTryCatch

And move the following definitions (declared elsewhere than
node_errors.h) to node_errors.cc:

  • Abort() (in util.h)
  • Assert() (in util.h)

Move the following code into a new node_errors.cc file and
declare them in node_errors.h for clarity and make it possible
to include them with node_errors.h.

- AppendExceptionLine()
- DecorateErrorStack()
- FatalError()
- OnFatalError()
- PrintErrorString()
- FatalException()
- ReportException()
- FatalTryCatch

And move the following definitions (declared elsewhere than
node_errors.h) to node_errors.cc:

- Abort() (in util.h)
- Assert() (in util.h)
@nodejs-github-bot nodejs-github-bot added 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. labels Nov 3, 2018
@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 3, 2018

V(v8) \
V(worker) \
V(zlib)
#define NODE_BUILTIN_STANDARD_MODULES(V) \
Copy link
Contributor

Choose a reason for hiding this comment

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

if this passes cpp-lint, maybe keep it with the old indent?

Copy link
Member

Choose a reason for hiding this comment

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

it's going to continue being a problem every time we run clang-format until someone lets it be merged. I say leave it.

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 This is done by make format-cpp, so the next time someone touches this bit it will be reindented again - I'd prefer we just go with whatever make format-cpp gives and forget about formatting issues, that'd be easier.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's my preference, the it will be fixed when there's a real change to the list.
But it's just a preference, non-blocking.

src/node_errors.cc Show resolved Hide resolved
src/node_errors.cc Show resolved Hide resolved
@joyeecheung
Copy link
Member Author

Ping: can I get some reviews? I'd like to land this one first so that I can include node_errors.h and reuse the code in another patch. If anyone is afraid this is going to block #23926 I could split out the bits that that patch also tries to move, but ideally I think we should just land this since that patch is still in progress and the (copy-pasted) refactoring should've been moved into one patch so that other patches can build on top of the reusable functions instead of waiting for a patch that actually tries to alter these functions.

@refack
Copy link
Contributor

refack commented Nov 6, 2018

IIUC #23926 still has some work to be done...

@joyeecheung joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2018
@joyeecheung
Copy link
Member Author

Landed in 5850220, thanks!

@joyeecheung joyeecheung closed this Nov 6, 2018
joyeecheung added a commit that referenced this pull request Nov 6, 2018
Move the following code into a new node_errors.cc file and
declare them in node_errors.h for clarity and make it possible
to include them with node_errors.h.

- AppendExceptionLine()
- DecorateErrorStack()
- FatalError()
- OnFatalError()
- PrintErrorString()
- FatalException()
- ReportException()
- FatalTryCatch

And move the following definitions (declared elsewhere than
node_errors.h) to node_errors.cc:

- Abort() (in util.h)
- Assert() (in util.h)

PR-URL: #24058
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
targos pushed a commit that referenced this pull request Nov 6, 2018
Move the following code into a new node_errors.cc file and
declare them in node_errors.h for clarity and make it possible
to include them with node_errors.h.

- AppendExceptionLine()
- DecorateErrorStack()
- FatalError()
- OnFatalError()
- PrintErrorString()
- FatalException()
- ReportException()
- FatalTryCatch

And move the following definitions (declared elsewhere than
node_errors.h) to node_errors.cc:

- Abort() (in util.h)
- Assert() (in util.h)

PR-URL: #24058
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
@refack refack added the landed label Nov 6, 2018
@codebytere
Copy link
Member

@joyeecheung would you want to backport this to v10.x?

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

6 participants