Do not use exceptions

[milabs]

No description provided.

[MartinBonner]

Could you give more details? Why not? What should the code do instead? Is this an option or universal?

[milabs]

Why using exception in that library? Qt doesn't use exceptions and works fine with error codes. You know, It's more complex than many parsers.. What to use instead of exceptions depends on API conventions. It may be function's return value or an status value reference passed onto the function. Anyway, it would be pretty much usable if we have an option for the library to use exceptions or not. At least for exception-less code.

[MartinBonner]

Interesting. It looks like a lot of the exceptions could be eliminated: For example, a better interface for setting settings in StreamWriterBuilder would mean we could refuse to set CommentStyle to anything other than "None" or "All". If we made duplicateStringValue use array new, rather than malloc, we could avoid our own "out of memory" exception. (And an application that wanted to be exception free would just have to cause new to terminate.)

Where I expected the exception was in operator[](const char*) (If the value is not a Object) - but that just asserts and aborts.

I think it would be a lot of work. (And a good design would be hard).

Edit: Fix markdown for operator []

[BillyDonahue]

You could run this argument in reverse. Users could wrap Jsoncpp in a private API that catches the exceptions it throws and returns errors in a style that "depends on API conventions" local to that user's app.

I think the exceptions are a rather natural fit for parsers. The parser code isn't cluttered with if/else handling and using up precious return types.

https://github.com/open-source-parsers/jsoncpp/search?utf8=%E2%9C%93&q=throw

The only uses of 'throw' are in:
jsoncpp/include/json/assertions.h

They are behind an #ifdef on JSON_USE_EXCEPTION, and cause 'abort()' if you don't have exceptions enabled. What exactly is the problem?

[MartinBonner]

I agree exceptions are a natural fit for a parser, but (eg) Reader::readValue calls throwRuntimeError( "Exceeded stackLimit in readValue()"). and throwRuntimeError just throws a RuntimeError.

There doesn't appear to be any ifdef on JSON_USE_EXCEPTION. Am I missing something?

(Note: I am looking at an amalgamated json.cpp)

[milabs]

Guys,

Exceptions are pain in the ass. Especially if every 3rd-party library tries to use own exception scheme. As for the the code, I don't understand completely of why someone wants to get an exception with Exceeded stackLimit in readValue(). message. OK, I've got it. And what? Doesn't it over-complicate the things? Why should you throw an exception if the function bool Reader::readValue() has bool type returned? If the stack overflow condition really occurs somehow, why not to return false and set lastError() to STACK_OVERFLOW.. ?

Same with throwRuntimeError("keylength >= 2^30");

As for the wrapping exception-full code to exception-less API, as mentioned by @BillyDonahue. I think that it's easy to add exceptions to exception-less code rather then remove exceptions form the exception-full one :-)

[milabs]

Also, things like:

  if (cs_str == "All") {
    cs = CommentStyle::All;
  } else if (cs_str == "None") {
    cs = CommentStyle::None;
  } else {
    throwRuntimeError("commentStyle must be 'All' or 'None'");
  }

tells that the exceptions used not for the right way. Why not to throw exceptions on every sneeze? :-)

[MartinBonner]

Except that we have exception-full code. Rewriting it to be exception free is quite a lot of work. Then you need to write the "add exception" wrapper to preserve compatibility with existing clients.

If you want an exception free library, I suggest you fork the repo, make the changes, and then (if you like) send Billy a pull request. If he decides not to accept the pull request, that's fine - you've still got your exception free library (and other people may prefer it too).

[MartinBonner]

"tells that the exceptions used not for the right way" - I don't see that. There are a number of sensible options here: abort, throw an exception, use a default. None of them are completely wrong.

I think my preference would be "abort", but I'm hardcore about "don't feed me garbage". (I really don't buy this "be permissive in what you accept" thing.)

[BillyDonahue]

[Sorry, I'm struggling with the lame GitHub code search feature here.]

There are some JSON_ASSERT that are really diagnostics for the library implementation's preconditions, and whether those throw or just assert/abort is behind an #if JSON_USE_EXCEPTION in assertions.h

jsoncpp/include/json/assertions.h

Line 20 in 8b9940f

#if JSON_USE_EXCEPTION

I would not want to see the code slowed down by error checking on Logic errors. Exceptions are faster than explicit checks for rare conditions.

The runtime errors are a different story. I think if we're already using a bool/lastError() scheme, we should stick to it consistently. If we're using a RuntimeException internally to manage the control flow for that condition to propagate, that might be fine, as long as they're caught and don't propagate up out of the the user API surface. The exceptions become an implementation detail.

[milabs]

@BillyDonahue The exceptions become an implementation detail. -- that is the worst thing. The scope of the project applicability is limited, indeed.

[BillyDonahue]

Please be specific. How is that "the worst thing"?

[MartinBonner]

I suspect milabs is working in an environment where they have a "no exceptions" rule and they don't even allow exceptions in third party code. That could happen in an old codebase which is still compiled with a "no exceptions" flag.

[cdunn2001]

operator new can throw bad_alloc. std::string::string can throw length_error. If you are using those functions, you are using exceptions. So purity is rather difficult in C++.

I personally hate exceptions. I also hate templates (because they compile slowly). I hate multiple inheritance. I hate operator overloading. I hate std::string. I could go on. Exceptions in particular are needed only in ctors, and only because ctors are doing real work, which they should not. But avoiding exceptions in ctors is difficult; you'd have to pre-allocate memory and pass that in. Anyway, we're not using Go or Ocaml, so I don't want to be pedantic. Let's consider some engineering trade-offs.

Reader is deprecated. So let's not talk about that class.

By the way, Qt has tons of classes. Yes, that is one way to store state flexibly. But in JsonCpp, whenever I add a class, people complain. Most people don't like the Builder, which I find invaluable for easy library maintenance and extensibility.

Anyway, let's consider dropping some specific exceptions. Consider StreamWriter* StreamWriterBuilder::newStreamWriter(). We could return NULL on illegal config. But std::string writeString(StreamWriter::Factory const& builder, Value const& root) uses. So we'd have to add cyclomatic complexity there, and then return an error code. But how? We could create a new class or two for maintaining a little extra state somewhere, maybe in the return object, or maybe in a an object used to help write. As I said, adding classes is really a non-starter. We could include an extra pointer parameter to return the error code. But what about std::ostream& operator<<(std::ostream& sout, Value const& root)? We could stop providing the operator overloads, but people like them. We could have exceptions in some API functions, but not in others. Ugh! Too hard to maintain. We could put the ostream into a bad state, but the problem is not really in the stream.

So I don't know what you're suggesting. And that's a relatively easy case. Elsewhere, without exceptions we would have to thread the error code up the call-stack. That's fine in Go, where the compiler double-checks your work, but in C++ we could easily overlook something. Yes, thorough testing would catch it, but it's easier to rely on the compiler for such things.

Exceptions are here for practical reasons. There are other json parsers. I highly recommend gason: super fast, minimal, easy to understand. JsonCpp fits a niche which may not be appropriate for you.

However, if someone issues a pull-request to turn the stack-overflow exceptions into !ok, I think that would be fine. Also in readObject() for length>2^30. There would still be exceptions in the helpers operator<</operator>> and writeString(), so somebody would need to document that. Is that fine?

Btw, the exception for stack-overflow is to avoid abort() or seg-fault, either of which would be a security hole, since the end-user would be able to crash the program. Error-code is fine; abort() is not. Like @MartinBonner, I'm fine with abort() for errors by the caller, but not for errors in the input (unless we also provide methods to help the caller validate input, which is non-trivial).

[cdunn2001]

Btw, I agree with comments from @BillyDonahue. In particular, exceptions can be very efficient and convenient in some contexts.

[cdunn2001]

A recent article measuring the speed of exceptions versus error codes:

• http://nibblestew.blogspot.fi/2015/12/are-exceptions-slower-than-error-objects.html

[robertbcalhoun]

Is it still possible to use jsoncpp without exceptions? After editing json.h as follows:

// If non-zero, the library uses exceptions to report bad input instead of C
// assertion macros. The default is to use exceptions.
#ifndef JSON_USE_EXCEPTION
#define JSON_USE_EXCEPTION 0
#endif

...I still get the same error about throwing exceptions when exceptions are disabled. (This is intentional; it's an embedded system).

src/External/jsoncpp.cpp: In function 'void Json::throwRuntimeError(const string&)':
src/External/jsoncpp.cpp:2658:25: error: exception handling disabled, use -fexceptions to enable
   throw RuntimeError(msg);
                         ^
src/External/jsoncpp.cpp:2659:1: warning: 'noreturn' function does return
 }

I'm building amalgamated source generated from master 264c3ed

Not a big deal, I can go back to gason. Just wondering whether this is by design or whether it is user error.

Thanks,
Rob

[cdunn2001]

I think the old JSON_USE_EXCEPTION macro might be lying now. But if you submit a patch to wrap all our exception throws with something based on that, we'd merge it.

I reiterate my endorsement of gason and encourage you to use that if it's right for you.

[BillyGoto]

Exceptions reduce complexity. What would you replace the exceptions with? The exceptional condition is still in the execution of the code so it would have to be reported back to you somehow or ignored. Just going from my own experience, where I would write code to handle a returned error code I can catch an exception just as (or more) easily. Catching exceptions can be harder than ignoring returned errors, but not harder than handling them.

On Wed, May 22, 2019 at 3:22 AM Kleenox ***@***.***> wrote: Please consider completely abandoning the use of exceptions for a future major version. We are in the process of removing jsoncpp in favor of rapidjson from our codebase out of the frustration caused by the use of exceptions, even though other than that I much prefer the simple way jsoncpp works, which is a real shame. Some projects are already technically complicated as it is and the last thing you need is to have to watch out for what should be a relatively trivial dependency throwing exceptions. Exceptions also make it unnecessarily harder to audit and proof code. Sure, you can adapt to it, but as I said, it's incredibly frustrating that an additional burden is being imposed on us by a trivial dependency. Sorry if this comes across harsh, just my two cents! — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#390?email_source=notifications&email_token=ABYXGPBBFOODT4WYO6OYM2TPWUGD7A5CNFSM4BVRQGA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV6OLBA#issuecomment-494724484>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABYXGPFBWXBCPULK3P5OAA3PWUGD7ANCNFSM4BVRQGAQ> .

-- ǝnɥɐuop ʎllıq

[baylesj]

I have ensured that the JSON_USE_EXCEPTION flag works appropriately, which is a requirement for the Chromium code base due to similar no exception policies. Generally speaking, dealing with exceptions is just part of working with modern C++ code, but we can maintain this somewhat hacky workaround in order to allow compat with projects using legacy compiler flags.

This issue should be fixed by PR #940

[milabs]

OMG, abort instead of error code that's hilarious IRLOL