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

Add parseIterator method to Json::Reader #75

Closed
wants to merge 1 commit into from
Closed

Add parseIterator method to Json::Reader #75

wants to merge 1 commit into from

Conversation

atomicptr
Copy link

Because this:

reader.parseIterator(vec.begin(), vec.end(), root);

is way more intuitive than:

reader.parse(&(*vec.begin()), &(*vec.end()), root);

@BillyDonahue
Copy link
Contributor

Is there any reason we shouldn't just have:

reader.parse(vec.begin(), vec.end(), root);

And have that work with any iterator including raw pointers?

/// \brief Parse from iterator
template<typename Iterator>
bool parseIterator(Iterator begin, Iterator end, Value& root, bool collectComments = true) {
return parse(&(*begin), &(*end), root, collectComments);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't going to work for any iterators other than contiguous-range pointers.
It will compile with them, but fail at runtime. We can't use this code.
It basically only works on vector or string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I'd rather have template code in a separate header when possible, to reduce compilation-time for those who do not need templates. But that's just a preference. Maybe compilers are getting faster?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there's a false economy there. The human engineering cost of adding
the definition when it's necessary becomes a point of brittleness. Nobody
thinks twice about including or . We should assume the
relatively tiny templates we need here are cheap enough to compile every
time.

Writing out full definitions for templates will also help diagnose
and eliminate ODR risks.

One way to get a little of the best of both worlds is with C++11 extern
templates, if you know the major template parameters to expect, like
append<const char*> in this case.

On Saturday, November 15, 2014, Christopher Dunn notifications@github.com
wrote:

In include/json/reader.h:

@@ -99,6 +99,12 @@ class JSON_API Reader {
/// \see Json::operator>>(std::istream&, Json::Value&).
bool parse(std::istream& is, Value& root, bool collectComments = true);

  • /// \brief Parse from iterator
  • template
  • bool parseIterator(Iterator begin, Iterator end, Value& root, bool collectComments = true) {
  • return parse(&(_begin), &(_end), root, collectComments);

Also, I'd rather have template code in a separate header when possible, to
reduce compilation-time for those who do not need templates. But that's
just a preference. Maybe compilers are getting faster?


Reply to this email directly or view it on GitHub
https://github.com/open-source-parsers/jsoncpp/pull/75/files#r20404843.

ǝnɥɐuop ʎllıq

@cdunn2001
Copy link
Contributor

C++11 extern templates

+1

@cdunn2001 cdunn2001 closed this Jan 24, 2015
@cdunn2001 cdunn2001 mentioned this pull request Jan 24, 2015
BillyDonahue pushed a commit to BillyDonahue/jsoncpp that referenced this pull request Dec 2, 2019
…seTweak

refactor structured error testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants