danielaparker / jsoncons

A C++, header-only library for constructing JSON and JSON-like data formats, with JSON Pointer, JSON Patch, JSON Schema, JSONPath, JMESPath, CSV, MessagePack, CBOR, BSON, UBJSON
https://danielaparker.github.io/jsoncons
Other
726 stars 164 forks source link

Problems with basic_json_diagnostics_visitor #385

Closed ecorm closed 2 years ago

ecorm commented 2 years ago

Usage of basic_json_diagnostics_visitor does not compile in C++11/14 modes due to its static constexpr CharT visit_foo_name[] members, which result in "unresolved external" linker errors.

The fix is to add the following below the class declaration:

    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_begin_array_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_end_array_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_begin_object_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_end_object_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_key_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_string_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_byte_string_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_null_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_bool_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_uint64_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_int64_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_half_name[];
    template <class C> constexpr C basic_json_diagnostics_visitor<C>::visit_double_name[];

It may be cleaner just to use traditional string literals instead directly in the std::cout statements.

With the above fix, when I run the example below, I get the following output, where there is no separation between the label the value:

visit_begin_object
visit_keyfoo
visit_uint6442
visit_end_object

Even with the above fix, I get the following compile error in Clang 12.0.0 (but not in GCC 9.4.0):

/home/emile/Dev/jsoncons/include/jsoncons/json_visitor.hpp:1399:14: error: 'jsoncons::basic_json_diagnostics_visitor<char>::visit_begin_array' hides overloaded virtual function [-Werror,-Woverloaded-virtual]
        bool visit_begin_array(std::size_t length, semantic_tag, const ser_context&, std::error_code&) override
             ^
main.cpp:7:30: note: in instantiation of template class 'jsoncons::basic_json_diagnostics_visitor<char>' requested here
    json_diagnostics_visitor visitor;
                             ^
/home/emile/Dev/jsoncons/include/jsoncons/json_visitor.hpp:954:14: note: hidden overloaded virtual function 'jsoncons::basic_default_json_visitor<char>::visit_begin_array' declared here: different number of parameters (3 vs 4)
        bool visit_begin_array(semantic_tag, const ser_context&, std::error_code& ec) override
             ^

The fix is to add the following missing override in basic_json_diagnostics_visitor:

        bool visit_begin_array(semantic_tag, const ser_context&, std::error_code&) override
        {
            std::cout << visit_begin_array_name << std::endl;
            return true;
        }

Might I also suggest that a basic_json_diagnostics_visitor constructor be added that takes an ostream reference to which the diagnostic info will be output. By default, it could be bound to std::cout to preserve the current behavior.

Include a small, self-contained example if possible

#include <string>
#include <jsoncons/json_parser.hpp>

int main()
{
    using namespace jsoncons;
    json_diagnostics_visitor visitor;
    json_parser parser;
    std::string input(R"({"foo":42})");
    parser.update(input.data(), input.size());
    parser.finish_parse(visitor);
    return 0;
}

What compiler, architecture, and operating system?

What jsoncons library version?

ecorm commented 2 years ago

Working on a PR for this.

ecorm commented 2 years ago

Ok. I understand now why "foo" style string literals where not used: it's because of the CharT template parameter. This is another reason why it shouldn't be hard-coded to output to std::cout.

danielaparker commented 2 years ago

Yes, I wrote the basic_json_diagnostics_visitor class specifically for testing the basic_json_visitor2 and basic_json_visitor2_to_visitor_adaptor classes during development, for receiving general CBOR and MessagePack key/value pairs and adapting them to JSON string/value pairs. It was never used in code that was tested with many compilers/environments, so its defects weren't caught.

And yes, the CharT template parameter complicates declaring string constants without excessive verbosity, and needing to support C++11 with less support for constexpr. Over the life of this project I went through a few attempts, and mostly settled on using static functions as shown here.

ecorm commented 2 years ago

Submitted PR #386 .

The reason why I stumbled upon the basic_json_diagnostics_visitor problems in the first place, is that I found a diagnostic visitor useful in troubleshooting the tests cases I made recently in my previous PRs. I had to write a temporary diagnostic visitor because the existing one wouldn't compile.

danielaparker commented 2 years ago

Or for constant C strings, like this

ecorm commented 2 years ago

Or for constant C strings, like this

I can change it in my PR so that it uses JSONCONS_CSTRING_CONSTANT.