Remove simdjson_move_result #628
Conversation
|
(I was toying with this idea anyway, given that |
f0ad8e7 to
3353df6
Compare
cc34782 to
ccee716
Compare
ccee716 to
56841bc
Compare
|
@jkeiser I am very, very confused .... why does jsonpointer (the tool) relies on deprecated functions? That's wrong, right? Didn't you just update it? |
|
@jkeiser Just so we have the same understanding... The && ref-qualifier means that the instance must be an rvalue. The & ref-qualifier means that the instance must be an lvalue. If we agree with this... then I don't quite understand your comments... because you have added the && ref-qualifier to tie... This means, as far as I can tell, that it can only be called on rvalues... |
I have explicitly not been changing the idiom of anything while deprecating stuff. All updates to the tools during deprecation have kept the same meaning (e.g. anything using ParsedJson::Iterator still uses it). Trying to keep separate things separate :)
@lemire You're right, but with one caveat: && is an rvalue reference. What this means, roughly, is "either a temporary value or a variable that will no longer be used." By passing you a reference with &&, the caller gives up all rights to use the object from that point on. See this reference for a fairly enlightening explanation. That's how it clicked for me, and the way I'm using && is backed up by the theory in there. It's new info to me, so please do tell me if you understand it differently after reading! |
|
@jkeiser My understanding is the same, I think. However, here is how I would name things... the ref qualifier & is for lvalues the ref qualifier && is for rvalues |
|
Ok. Freeze your PR as it stands, I will patch it so that it passes the tests. |
|
Yeah, I think I got lvalues / rvalues wrong in my initial explanation. I've fixed the description. So do you feel && is in the right places, and will work well for users, based on your understanding of it? |
|
(PR is frozen.) |
|
@jkeiser I think that the code is correct and I now think that the description is correct. |
|
I still have a way to go, but I think it is necessary for us to remove deprecated calls from our own tools. |
I agree. We should also find a way to turn off the deprecation warnings for tests that are explicitly testing deprecated APIs, without turning them off for everything. My suggestion: comment out the warnings-as-errors tests in this PR, and we can attack this problem separately and then enable them. Either way, I think it needs to be deprecation-clean by 0.3. |
|
I have upgraded all of our tests and tools to the new API. I am now going to wait to see whether it runs without any warning in recent GCC compilers. |
Too late. :-) |
|
@jkeiser I suggest you have a look at my changes. I tweaked your API a bit. |
|
@jkeiser This passes all tests now. Merge at will. |
|
(Any and all of these comments can be cleaned up later, and I may do so before merging anyway.) |
8d7ddd7 to
f0f111b
Compare
- Remove const char * variants for at_key(), string_view covers them - Add at_key_case_insensitive variants on *_result - Add at(), at_key(), at_key_case_insensitive() tests
|
@lemire if you could give a once-over on my last 4 commits (should be reasonably small), I'll merge this puppy (after tests of course). |
|
@jkeiser I scanned everything new and I don't find anything objectionable. Merge at will. |
|
CircleCI is painfully overloaded at the moment, most jobs are queued but not running. |
This uses the C++ lvalue operator to enforce moves in and out of simdjson_result, allowing us to use a single class for owned and non-owned things. We never call std::move() anymore, instead relying on
forwardto figure out whether to copy (in the case of references) or move (in the case of values).Specifically:
tie()can only be done on an rvalue now (i.e. once you tie() you have to throw away the simdjson_result).operator Tis changed tooperator T&&and requires the simdjson_result to be an rvalue.T&&, requiring the input to be an rvalue.With that, we now use simdjson_result<padded_string> for load() and can remove simdjson_move_result entirely.
Fixes #622