Convert simdjson to use .get() - #960
Conversation
86fdcec to
0c9dc11
Compare
| stat_t s{}; | ||
| simdjson::dom::parser parser; | ||
| auto [doc, error] = parser.parse(p); | ||
| simdjson::dom::element doc; |
There was a problem hiding this comment.
Here we see a bit of a negative: the user needs to know about simdjson::dom::element which wasn't the case before.
Are we keeping the old API ( auto [doc, error] = parser.parse(p);)? I think that some users will prefer it.
I am "old school" and I prefer code that is explicitly typed. I don't like magic and surprises, but I can imagine people being annoyed at having to know the type that they need.
There was a problem hiding this comment.
(And I understand the point about coding for C++11.)
There was a problem hiding this comment.
Yeah. I think there is likely a large intersection between users of error codes and explicit-type-lovers, but it's real :)
I find myself liking this because I'm not actually a huge fan of auto in the parser.parse() case. For me, the guideline is "omit the type iff it's obvious." The use of auto is for a sub-rule of that: rule "only write the type once per line, because it's obvious."
I think I'm a reasonable example of the user who hates typing unnecessary things (for many reasons). I went so far as to design a language and compiler partly around that hatred :)
| case dom::element_type::STRING: { | ||
| stat.stringCount++; | ||
| std::string_view sv = v.get<std::string_view>(); | ||
| auto sv = std::string_view(v); |
There was a problem hiding this comment.
The cast still requires exceptions, which bothers me. I am just not sure how to make it better.
I just realized it could have been std::string_view sv = v; which has better optics IMO. Gonna change that.
| simdjson::error_code allocate_error = parser.allocate(1000*1000); // This allocates enough capacity to handle documents <= 1MB | ||
| if (allocate_error) { cerr << allocate_error << endl; exit(1); } | ||
| auto error = parser.allocate(1000*1000); // This allocates enough capacity to handle documents <= 1MB | ||
| if (error) { cerr << error << endl; exit(1); } |
There was a problem hiding this comment.
(I wonder whether exit(error) would ever make sense?
There was a problem hiding this comment.
(Minor point, more a note for myself.)
There was a problem hiding this comment.
It could. I've learned to be careful with overallocating exit codes early, though. People end up relying on them in ways that constrain future changes :)
| * dom::parser parser; | ||
| * parser.parse(R"({ "a\n": 1 })")["a\n"].get<uint64_t>().value == 1 | ||
| * parser.parse(R"({ "a\n": 1 })")["a\\n"].get<uint64_t>().error == NO_SUCH_FIELD | ||
| * parser.parse(R"({ "a\n": 1 })")["a\n"].get<uint64_t>().first == 1 |
There was a problem hiding this comment.
The switch to first/second is somewhat unfortunate (as opposed to value(), error()) because it forces the user to remember who is on first and who is on second. (joke)
There was a problem hiding this comment.
But thinking about it, you'd probably not want to ever call it like so.
There was a problem hiding this comment.
I'm not happy about .first, but it's not really something we want people to use outside of tests like this ... it's a method to ignore errors when exceptions are off.
There was a problem hiding this comment.
What is wrong with...
parser.parse(R"({ "a\n": 1 })"_padded)["a\n"].get<uint64_t>().value();
???
There was a problem hiding this comment.
What is wrong with...
parser.parse(R"({ "a\n": 1 })"_padded)["a\n"].get<uint64_t>().value();???
.value() presently only exists when exceptions are off, because it throws an exception if there was an error. We could totally make it work for non-exception cases by removing the exception throw and treating it like it means .assume_no_error(). When I built value() a while back, I was being cautious about introducing methods that help you ignore errors.
There was a problem hiding this comment.
When I built value() a while back, I was being cautious about introducing methods that help you ignore errors.
That sounds reasonable.
There was a problem hiding this comment.
I am pretty sure that the documented examples of the form...
parser.parse(R"({ "a\n": 1 })")["a\n"].get<uint64_t>().firstare not what you mean. For one thing, I am not sure it would compile. For another, I don't understand why you would replace "value()" by "first".
|
Because .value() isn't available with exceptions disabled. I did a search for .value() and replaced it everywhere. |
|
I could imagine making .value() be the "unsafe version that assumes there is no error," though. It would make these examples better and make sense ... I think. I've been trying to be cautious introducing a supported ability to ignore errors. |
|
(I am also ok with changing the documented examples back until we decide on the right form for .assume_no_error().) |
|
@jkeiser Let me be clearer.... I don't think that this will even compile... parser.parse(R"({ "a\n": 1 })")["a\n"].get<uint64_t>().first == 1I don't mind so much the first if that's what we want to go with as a documented example, but you have to pass a padded_string or something. |
|
Good catch! Yeah, it needs _padded. Independent of first or value though. |
Correct. |
lemire
left a comment
There was a problem hiding this comment.
Merge subject to change so that short examples are valid code (operating on padded strings).
|
Examples fixed. Doc only. Merging without validating CI. |
This PR takes converts almost all the following to use `error = x.get(v):
.get<T>()auto [x,error] =.tie(x,error)Aside from tests that explicitly test them. The only thing it actually leaves around is a few .get()'s in tests, and the
for (auto [key,value] : object)pattern. That has value, but we might want to stop using it idiomatically internally just so all our code is C++11 clean. It's better, but not so much better that it's worth the trouble.The main goal here is to teach users one way to do error codes (
e = .get(v)) and one way to do exceptions (T()/T v =). There may be more than one way to do it, but the more ways you present, the less "choice bandwidth" people have to spend on other stuff. And .get(v) is pretty damn ergonomic, for C++!