Add no thrown exception support to parse #798

jseward · 2017-10-24T08:58:05Z

This PR adds support for parse to not throw exceptions. Use similar pattern as std::experimental::filesystem where overload that takes in std::error_code& does not throw an exception.

Did not add new tests as changes to existing tests provide full coverage of new codepath.

NOTE: accidentally branched from #795, ignore that for now.

This avoids unnecessary string copies on often used find().

Copied from solution to nlohmann#464

@gregmarr

Instead implement @gregmarr's PR comments that perfect forwarding should be used. Also cleaned up cpp language standard detection.

Already covered by AppVeyor

This allows users to parse json without raising exceptions but still get full exception details. This is very similar to how std::networking and std::filesystem work. In that if the overload with std::error_code& is used then no exceptions are raised. - Removed static create functions from exceptions. Replaced with normal constructor so that it can be created on heap for assignment to std::unique_ptr. - Removed bool allow_exceptions, no longer necessary with the &ex overload.

theodelrieu · 2017-10-24T12:07:00Z

test/src/unit-class_parser.cpp

@@ -45,7 +45,8 @@ json parser_helper(const std::string& s)
 // if this line was reached, no exception ocurred
 // -> check if result is the same without exceptions
 json j_nothrow;
- CHECK_NOTHROW(json::parser(nlohmann::detail::input_adapter(s), nullptr, false).parse(true, j_nothrow));
+ std::unique_ptr<json::exception> ex;


What is the advantage of a unique_ptr over std::exception_ptr?

std::exception_ptr isn't limited to json::exception types.

The only things you can do with an std::exception_ptr are convert to bool, compare to another pointer, and rethrow. You can't actually get the exception object out of it, so all the caller would be able to do is determine whether or not an exception was thrown, not what that exception was.

Right, there's no way to retrieve the exception, forgot about that.

I do think we should not use std::unique_ptr<json::exception> though, but std::error_code instead (this blog post is really useful on this topic).

std::filesystem uses std::error_code for the noexcept overloads, one can still embed a lot of information with this facility. I've seen it mentioned in Outcome's documentation too.

What do you all think?

I don't work without exceptions, so I don't really care one way or another. I think the best solution in the end is probably to return an std::expected or std::outcome or whatever it ends up as, but that's years away.

I agree that long term using std::expected or std::outcome would be best. My original plan was to use std::error_code (as you can see by my branch name) but it has it's own issues. Mostly creating a custom formatted error message with context does not seem cleanly possible. You can see in the blog post linked (which I used as a source as well) that aspect is glossed over.

I definitely see where you are coming from @theodelrieu and pretty much agree with every one of your points. I appreciate the links.

The issue is in regards to a specific, but I would argue potentially popular, scenario though. That is using this library for parsing json and getting nice context aware error messages on failures even if exception handling is completely disabled. (most common in game engines, embedded)

Being able to handle various error codes / categories at runtime for parse errors is not important here, and I can't see the ability to do so being useful other than a theoretical exercise.

@nlohmann does the above description answer your question on why this PR exists?

So basically all you want to have is access to parser diagnostics (error message + location) when exceptions are switched off?

@jseward I understand the use-case, I think a compromise can be reached.

The good solution would be outcome, you can indeed attach an extra payload to it. Unfortunately, I do not think we can use it in this library.

However, we could have the following type:

// not a good name (std::system_error is used as an exception). struct json_error { std::error_code ec; std::string context; // empty if ec == 0, or no context provided. };

This type would replace the std::unique_ptr<json::exception> in this PR. Plus, the #ifdef inside json::exception definition can be removed.

I would also suggest adding an error_code to json::exception too (for the same reasons mentioned before in this thread, similarly to std::filesystem_error).

What do you all think?

We already support parsing without exceptions (and return a discarded type on parse errors). I think the desired outcome could be achieved by just adding the required member functions to the parser class to query the required diagnostics in case an error occurs. The same functions could be re-used in the code that creates the actual parse_error exception so we would not even need to add too much code.

theodelrieu · 2017-10-24T12:09:17Z

src/json.hpp

 */
 void parse(const bool strict, BasicJsonType& result)
 {
+ std::unique_ptr<exception> ex;
+ parse(strict, result, ex);
+ try_throw_any_exception(ex.get());


std::rethrow_exception could be used here (assuming we replace ex with a std::exception_ptr.

ah interesting thanks! I actually have never heard of std::exception_ptr before. Unfortunately it looks like it isn't very useful in this situation. Specifically I want to be able to get at the message on why the parse failed.

gregmarr · 2017-10-24T14:08:20Z

src/json.hpp

 };

+/*!
+@brief helper function to throw the a downcasted exception.


Typo: "throw the a downcasted" too many articles

gregmarr · 2017-10-24T14:19:58Z

src/json.hpp

+ ex = ex; // suppress unreferenced formal parameter warnings
+ std::abort();
+#endif
+}


I think this looks cleaner

#ifdef JSON_EXCEPTIONS_ENABLED template<class Exception> void try_throw_exception(exception* ex) { // Only use RTTI if necessary (exceptions are enabled). This way // can be compiled with no RTTI. if(Exception* down = dynamic_cast<Exception*>(ex)) { JSON_THROW(*down); } } #else template<class Exception> void try_throw_exception(exception* /*unused*/) { std::abort(); } #endif

On the other hand, why is this even needed? (It's definitely not if @theodelrieu 's suggested change is made, and I would say it's not even if it isn't, just throw ex.get(), no need for the downcasting.

Agreed, that does look cleaner thanks. I'll try it out. As for just throwing without downcasting I couldn't get that to work. catch (const SpecificError& e) {} would never get hit.

Yeah, I missed that you were throwing by value instead of by pointer, which unfortunately does require all this extra casting over throwing directly when the exception is created. This delayed throw method does lose some of the call stack context, so it may be better to make the choice whether to throw or place in the unique_ptr<> at the point of the THROW macro.

- fix comment - clean up #ifdef organization based on PR feedback (agreed looks better)

Forgot not available for C++11

coveralls · 2017-10-25T08:27:22Z

Coverage decreased (-0.03%) to 99.966% when pulling 6fbe924 on jseward:add_error_code into 1b1bd0e on nlohmann:develop.

trilogy-service-ro · 2017-10-25T13:34:35Z

Can one of the admins verify this patch?

coveralls · 2017-10-26T08:33:01Z

Coverage remained the same at 100.0% when pulling b9c8842 on jseward:add_error_code into 1b1bd0e on nlohmann:develop.

nlohmann · 2017-10-27T09:40:23Z

Hey there. I am a bit confused about this PR. Can someone please provide a brief explanation on the benefits?

stale · 2017-11-27T10:47:41Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

vinniefalco · 2017-12-04T15:12:27Z

LOL wtf? A stale bot?

jseward added 15 commits October 21, 2017 15:20

Add string_view support

1a66679

This avoids unnecessary string copies on often used find().

Fix for _HAS_CXX17 == 0

59cde1a

Copied from solution to nlohmann#464

Use consistent logic to determine if string_view exists

d468f8c

Remove JSON_HAS_STRING_VIEW

33c6511

Instead implement @gregmarr's PR comments that perfect forwarding should be used. Also cleaned up cpp language standard detection.

Remove tabs for spaces

73b1629

Remove c++17 support flag in cmake

16ffdbc

Already covered by AppVeyor

Remove old non-perfect forwarding find and count

6c9a401

Remove extra spaces

715c98b

Add missing spaces

992c836

Fix bugs found in test-class_parser

720d86d

Add JSON_USE_RUNTIME_ERROR_STORAGE

f6c1ab6

Add JSON_EXCEPTIONS_ENABLED

5fc084d

Add RTTI disabled logic

a18eef8

Remove tabs for spaces

0207d7e

jseward changed the title ~~Add error code~~ Add no exception support to parse Oct 24, 2017

jseward changed the title ~~Add no exception support to parse~~ Add no thrown exception support to parse Oct 24, 2017

theodelrieu reviewed Oct 24, 2017

View reviewed changes

gregmarr reviewed Oct 24, 2017

View reviewed changes

jseward added 2 commits October 24, 2017 23:25

Improve try_throw_exception

4d349ab

- fix comment - clean up #ifdef organization based on PR feedback (agreed looks better)

Remove std::make_unique

6fbe924

Forgot not available for C++11

Add necessary LCOV_EXCL_LINE

b9c8842

stale bot added the state: stale the issue has not been updated in a while and will be closed automatically soon unless it is updated label Nov 27, 2017

stale bot closed this Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add no thrown exception support to parse #798

Add no thrown exception support to parse #798

jseward commented Oct 24, 2017 •

edited

Loading

theodelrieu Oct 24, 2017

gregmarr Oct 24, 2017

theodelrieu Oct 24, 2017

gregmarr Oct 24, 2017

jseward Oct 25, 2017

jseward Oct 28, 2017

nlohmann Oct 28, 2017

jseward Oct 28, 2017

theodelrieu Oct 28, 2017

nlohmann Oct 28, 2017

theodelrieu Oct 24, 2017

jseward Oct 25, 2017

gregmarr Oct 24, 2017

gregmarr Oct 24, 2017

jseward Oct 25, 2017

gregmarr Oct 25, 2017

coveralls commented Oct 25, 2017

trilogy-service-ro commented Oct 25, 2017

coveralls commented Oct 26, 2017

nlohmann commented Oct 27, 2017

stale bot commented Nov 27, 2017

vinniefalco commented Dec 4, 2017

Add no thrown exception support to parse #798

Add no thrown exception support to parse #798

Conversation

jseward commented Oct 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 25, 2017

trilogy-service-ro commented Oct 25, 2017

coveralls commented Oct 26, 2017

nlohmann commented Oct 27, 2017

stale bot commented Nov 27, 2017

vinniefalco commented Dec 4, 2017

jseward commented Oct 24, 2017 •

edited

Loading