Rapidjson: Particular and general use of assert()

[crypticmind]

In Particular

Why is there an assert(false) line in document.h, line 1103?

As far as I understand, there's the issue of clang's exit-time destructors that was overcome by the use of a heap-allocated object, but I don't understand why is there an assert aborting the execution?

In General

I feel many instances of assert() are being used to validate client usage rather than program pre-/post-conditions or invariants, e.g. calling AddMember() on a value that wasn't created passing the kObjectType flag.

I'd like to know the reasoning behind this decision, and whether it'd be possible/make sense to turn the assertions into exceptions that can be handled in a nicer way.

[miloyip]

In Particular

There is no way to properly handle this situation with this operator overloading. It can be analogue to index out of bound in array. The member must be exist.

Contrary to std::map, the current design cannot create a key-value pair when the key does not exist. Since it will need an allocator for that, and that will make the API absurd.

In General

For a dynamic typing kind of data structure, some operations are only valid for a particular JSON type. The user need to check wether the type of a value is valid before using such operations. This can be ensured by writing proper validation code. You can also use JSON Schema so that less hand-written validation code is needed.

To make RapidJSON usable in most situations, we chose not to use exception.

Besides, I am planning to create a more modern C++ version of DOM, which will be co-exist with the current one. Feel free to give suggestions.

[crypticmind]

So, it makes sense to cancel the operator[] execution if the key doesn't exists then, and the code aborts instead of throwing by design.

Would it make sense to replace asserts with exceptions with a flag? I understand you're planning a new C++ version, but you're referring only to the DOM component. The flag I'm asking about would be library-wise.

[OlafvdSpek]

I'm not sure that'd make sense.. it's a design / logic error (bug!) in your program right?

[pah]

You can already configure RapidJSON to use exceptions by defining certain macros before including RapidJSON headers (e.g. in a local customization header).

For parse errors, see RAPIDJSON_PARSE_ERROR_NORETURN.

For assertions (i.e. logic errors, as @OlafvdSpek pointed out), see AssertException and the following definition of RAPIDJSON_ASSERT in the unit test setup.

[crypticmind]

@pah Looks good! I'll check it out.

@OlafvdSpek You're right. What drives me towards exceptions is that I prefer a client to tell me that _one API doesn't work_, rather than _one API kills the server_.

[OlafvdSpek]

RapidJSON doesn't provide that kind of managed API by default, as it usually affects performance.

[crypticmind]

OK, I'll guess I'll have to pay attention to wherever the library may abort.
You can close this issue.

[OlafvdSpek]

You could write a wrapper that checks before accessing a member and takes appropriate action if it doesn't exist.

For example:

inline const rapidjson::Value* find_ptr(const rapidjson::Value& v, const char* k)
{
    if (!v.IsObject())
        return NULL;
    if (const char* dot = strchr(k, '.'))
    {
        auto p = find_ptr(v, string(k, dot - k).c_str());
        return p && p->IsObject() ? find_ptr(*p, dot + 1) : NULL;
    }
    auto p = v.FindMember(k);
    return p == v.MemberEnd() ? NULL : &p->value;
}

inline const char* get_string(const rapidjson::Value& v, const char* k)
{
    auto p = find_ptr(v, k);
    return p && p->IsString() ? p->GetString() : "";
}

[crypticmind]

Yeah, I was sort of heading that way.

To serialise back and forth, I created mappers between basic types that can be used to create more complex mappers, and the basic type mappers already check before accessing stuff.

I was creating a mapper for std::optional when I faced this issue.

Thank you for the help!