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

Support startIndex for from_cbor/from_msgpack #462

Closed
prsyahmi opened this issue Feb 19, 2017 · 18 comments
Closed

Support startIndex for from_cbor/from_msgpack #462

prsyahmi opened this issue Feb 19, 2017 · 18 comments
Assignees
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Milestone

Comments

@prsyahmi
Copy link

prsyahmi commented Feb 19, 2017

The idea is to not create another copy of std::vector since my cbor data start at certain offset.

    static basic_json from_cbor(const std::vector<uint8_t>& v, const size_t startIndex = 0)
    {
        size_t i = startIndex;
        return from_cbor_internal(v, i);
    }
@nlohmann
Copy link
Owner

Done.

@prsyahmi
Copy link
Author

Thanks!

@nlohmann nlohmann added the aspect: binary formats BSON, CBOR, MessagePack, UBJSON label Mar 28, 2017
nlohmann added a commit that referenced this issue Aug 16, 2017
The CBOR and MessagePack parsers now expect the input to be read until the end. Unless the new parameter "strict" is set to false (it is true by default), an exception is raised if the parser ends prematurely. This is a breaking change as the parsers ignored unread input so far.

Furthermore, the offset/startIndex paramter introduced in #462 was removed as this behavior can be mimicked with an iterator range. For instance, instead of calling "from_cbor(vec, 5);", you can write "from_cbor({vec.begin()+5, vec.end()});".
@nlohmann
Copy link
Owner

With 22b5969, the offset/startIndex parameter was removed as this behavior can be mimicked with an iterator range. For instance, instead of calling

json j = json::from_cbor(vec, 5);

you can write

json j = json::from_cbor({vec.begin()+5, vec.end()});

@theodelrieu
Copy link
Contributor

Are the braces {} intentional?

@nlohmann
Copy link
Owner

Yes, I did not find a better way to implicitly create an input adapter from two parameters. And since there is a parameter with a default value following, I cannot use variadic parameters.

(Though I would love to find a solution!)

@nlohmann nlohmann added the solution: proposed fix a fix for the issue has been proposed and waits for confirmation label Aug 16, 2017
@theodelrieu
Copy link
Contributor

theodelrieu commented Aug 16, 2017

I didn't go into the details of input_adapter, but what about adding a new overload of from_{cbor,msgpack} that take an iterator range, just like parse?

You could then construct the input_adapter as you did in your example code.

@nlohmann
Copy link
Owner

That would be one solution, but then I also have an input adapter to parse from a uint8_t* and a length, and then I would need another family of overloads...

@nlohmann
Copy link
Owner

(Of course, the example above could be written as

json j = json::from_cbor(json::input_adapter(vec.begin()+5, vec.end()));

@theodelrieu
Copy link
Contributor

Actually, this is also supported by parse:

auto j = json::parse(vec.begin(), vec.end());
auto j2 = json::parse(vec.data(), vec.data() + vec.size());

Both cases are covered with the same overload. Plus, that would be symmetrical with other parts of the API (e.g. parse as mentioned).

If those kind of methods could share the same API, that would become really intuitive to use IMO.

@nlohmann
Copy link
Owner

I know that this is desirable, but I don't really like copy/pasting so many functions...

@nlohmann
Copy link
Owner

On the other hand, there are at most two parameters, so I can add two functions for each parser and forward the arguments to the input_adapter constructors.

@nlohmann
Copy link
Owner

@theodelrieu Since you are the SFINAE wizard - what do you think of

template<typename A1, typename A2,
         detail::enable_if_t<std::is_constructible<detail::input_adapter, A1, A2>::value, int> = 0>
static basic_json from_cbor(A1&& a1, A2&& a2, const bool strict = true)
{
    return binary_reader(detail::input_adapter(std::forward<A1>(a1), std::forward<A2>(a2))).parse_cbor(strict);
}

@theodelrieu
Copy link
Contributor

Ahah I'm not sure I deserve that title :)

Well that will work, but I think it's quite brittle for some reasons:

  • Too complex for what it does.

  • You have no idea what kind of arguments the function takes. (Not to mention that input_adapter is not exposed, due to the detail namespace)

  • Might break if there is a new ctor to input_adapter (or one less)

I understand that you don't want to copy code, in order to get the same interface than parse, but that's really what we want IMO:

All those functions (parse/from_cbor/msgpack) only take byte containers or equivalent.

Last but not least, I'd rather write boilerplate in the library code than in the client code, all day everyday! Even more when it's just a few lines ;)

@nlohmann
Copy link
Owner

I wish I had some arguments that do not translate into "I am too lazy to do the copying...". :-/

@nlohmann
Copy link
Owner

@theodelrieu
Copy link
Contributor

I can take care of it :p

Anyway I'd rather not have to rebase my PR (#700) twice a day, so if you want to wait a bit before committing that's fine for me!

@nlohmann
Copy link
Owner

I won't touch this one right now. I'm about to finish #593 and then I let the library go.

@nlohmann
Copy link
Owner

About #462 (comment):

I just realized we do more or less the same here: https://github.com/nlohmann/json/blob/develop/src/json.hpp#L8368

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aspect: binary formats BSON, CBOR, MessagePack, UBJSON kind: enhancement/improvement solution: proposed fix a fix for the issue has been proposed and waits for confirmation
Projects
None yet
Development

No branches or pull requests

3 participants