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

Make throw.*Error print msg when built with JSON_USE_EXCEPTION=0 #1150

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

bcsgh
Copy link
Contributor

@bcsgh bcsgh commented Mar 24, 2020

Tested: used this to debug an issue in my code.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.858% when pulling c334d61 on bcsgh:abort-message into 3beb37e on open-source-parsers:master.

dota17
dota17 previously requested changes Mar 30, 2020
JSONCPP_NORETURN void throwRuntimeError(String const& msg) { abort(); }
JSONCPP_NORETURN void throwLogicError(String const& msg) { abort(); }
JSONCPP_NORETURN void throwRuntimeError(String const& msg) {
std::cerr << msg << std::endl;
Copy link
Member

Choose a reason for hiding this comment

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

I'm afraid noreturn function should not return anything.

Copy link
Contributor Author

@bcsgh bcsgh Mar 30, 2020

Choose a reason for hiding this comment

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

I'm not seeing the issue. These functions don't return.

They do have side-effects before aborting, but then there is nothing wrong with a no-return function having side effects.

Edit: If I'm missing your point, could you please be more specific about what changes you want?

Copy link
Contributor

Choose a reason for hiding this comment

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

We are a library and we don't control std::cerr. We must not reach into the app and decide what information should be put to std::cerr. The app might have a completely different logging system to which this error should be put. It's an app-level decision. The most we should do is allow for a user-defined error handling callback. Then if they want to write to std::cerr they can install the behavior to do so, but we should not be making that decision for them.

@cdunn2001 cdunn2001 dismissed dota17’s stale review April 24, 2020 18:40

I think this is ok. Since the program is about to abort(), all bets are off. Might as well write to stderr first, since this is a hard-crash, already a security proble.

@cdunn2001 cdunn2001 merged commit 91f1553 into open-source-parsers:master Apr 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants