Skip to content
New issue

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

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

Already on GitHub? # 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.

@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?

# for free to join this conversation on GitHub. Already have an account? # 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