Making `tag_invoke` a "put" as opposed to a "get" by the-moisrex · Pull Request #2256 · simdjson/simdjson

the-moisrex · 2024-09-18T18:21:17Z

Previously the tag_invoke would be used as:

struct Car {
  // ...

  friend simdjson_result<value> tag_invoke(tag_t<deserialize>, type_identity<Car>, auto& val) {
    Car car{};
    // ...
    return car;
  }
};

But now it's going to be used like this:

struct Car {
  // ...

  friend error_code tag_invoke(tag_t<deserialize>, auto& val, Car& out) {
    // ...
    // instead of doing
    //   out = ...
    // we do
    //   out.this = that
    //   out.that = other
    // the user can choose to do any of them,
    // but in the past, the user was limited to the former way.
    return SUCCESS;
  }
};

This allows for this syntax if #2247 gets merged:

Car mine, hers;
// Add default values to the cars
mine.name = "ME";
hers.name = "HER";
error = obj.extract(
  to{"my_car", mine},
  to{"her_car", hers}
);
if (error) {
  return error;
}

Now this doesn't replace the defaults anymore; only the fields that are already present in the JSON string is going to be replaced.

This also can be used to push_back to a vector instead of replacing the whole vector.

Changes

tag_invoke stuff mentioned above
Fix nlohmann ambiguity on C++23-enabled clang
Introducing deserializable and nothrow_deserializable concepts
Making document/value::get() dependent on document/value::get(T& out) instead of the other way around.
T in document/value::get<T>() needs to be Default Constructible now.

I did initially wanted to make document/value::get<T>() to support arguments (document/value::get<T>(Args...)) so instead of default constructible, we could pass allocators and what not to it, but I felt it would make ::get a very confusing API.

And I only kept the new API in ::get(T& out) and not something else like ::into(T& out) because the current way is done in that way, but ::get(T& out) is definitely a confusing verb for the job.

P.S.: turns out this bug was due to the old dependencies/.caches caches. It would be better if those caches are put into the build directory so removing the build directory would be equivalent of removing all the caches. Wasted so much debugging the wrong things because of this until I found out what I was seeing on cxxopts github differs from the cache files.

* fix: add tests related to issue 2227 * avoiding name clash * pedantic fix * deprecate rvalue get on document * selectively deprecating

* fix ndjson spec link The link in the readme of parse_many links to a casino spam site * fix link

With singleheader on, clangd can't find the right include files.

Reduce errors in vscode

…imdjson#2240)

With gcc it causes an error in `simdjson.cpp`: ``` simdjson.cpp:548:9: warning: #pragma once in main file 548 | #pragma once | ^~~~ ``` It had previously been commented out in: simdjson@6ef555e However, this was lost in an upgrade: simdjson@2a4ff73

* adding missing undef silencer * Updating CI * more fixes * fix * big endian fix

lemire · 2024-09-18T18:38:23Z

@the-moisrex You could sync with our main branch if you want the CI to go green. (Optional.)

lemire · 2024-09-18T18:41:25Z

turns out #2235 (comment) was due to the old dependencies/.caches caches. It would be better if those caches are put into the build directory so removing the build directory would be equivalent of removing all the caches. Wasted so much debugging the wrong things because of this until I found out what I was seeing on cxxopts github differs from the cache files.

We can improve the management of our dependencies. I will prepare a PR.

…ilder_development_branch_extra `document::get()&&` is now deprecated

the-moisrex · 2024-09-18T19:03:55Z

Merged master into it.

lemire · 2024-09-18T19:34:12Z

@the-moisrex

See PR #2257 regarding dependencies.

Note that it is still possible for things to get messed up with CPM. No dependency management system is ever perfect. But CPM is much more configurable.

lemire · 2024-09-21T00:05:00Z

I don't think we can merge this PR. As it stands, it changes over 30 files and 800 lines of code. It is difficult to review.

lemire

This PR changes 49 files and it is difficult to understand why some files are changed (e.g., this PR updates the .github/workflows files).

lemire · 2024-09-21T00:13:11Z

@the-moisrex Note that we have not merged the tag_invoke work into master, it is in builder_development_branch.

(We are not yet ready to release the builder work.)

lemire · 2024-09-21T00:14:15Z

@the-moisrex I recommend breaking this up into small management pieces and doing PRs against builder_development_branch.

https://github.com/simdjson/simdjson/tree/builder_development_branch

…into builder_development_branch_extra" This reverts commit 3eeecba, reversing changes made to 6858b20.

the-moisrex · 2024-09-21T09:25:46Z

@lemire I reverted the merge; it went back to 11 files changed before the merge from master.

lemire

I am switching my recommendation to 'approve' PR.

We do invite review from the community.

lemire · 2024-09-24T18:47:52Z

Ok. I am merging this into simdjson:builder_development_branch.

@lemire

* tag_invoke based custom types (#2219) * tag_invoke based custom types Now you can use tag_invoke to add a custom type or a group of custom types. * Fixing macro usage + Fixing noexcept * Fixing the usage of #include We don't need <concepts> at all seems like it * Fixing tag_invoke impl for MSVC * Making `tag_invoke` to support `ondemand::document` as well + docs (#2228) * Making `tag_invoke` to support `ondemand::document` as well + docs * Fix typos and doc update by @lemire Co-authored-by: Daniel Lemire <daniel@lemire.me> * Better docs by @lemire Co-authored-by: Daniel Lemire <daniel@lemire.me> * Preserving the old, disallowing in the new I'm disabling `document::get() &&` if the user has provided a `tag_invoke`d version; otherwise, we retain the compatibility. --------- Co-authored-by: Daniel Lemire <daniel@lemire.me> * fix: correct small issues with deserialize (#2232) * Extending the deserialization code with more defaults + docs (#2233) * Make custom types easier with some predefined cases + docs * missing include * adding Ubuntu 24 CXX 20 * using concepts all the way * minor tweak * tiny tweak * tweaks * more tweaking * saving --------- Co-authored-by: Daniel Lemire <dlemire@lemire.me> * Making `tag_invoke` a "put" as opposed to a "get" (#2256) * fix: add tests related to issue 2227 (#2229) * fix: add tests related to issue 2227 * avoiding name clash * pedantic fix * deprecate rvalue get on document * selectively deprecating * Fix ndjson spec link (#2234) * fix ndjson spec link The link in the readme of parse_many links to a casino spam site * fix link * [no-ci] Update README.md * Make simdjson compile again * Enable SIMDJSON_SINGLEHEADER=OFF in VS Code With singleheader on, clangd can't find the right include files. * Add missing include directives to static build targets of simdjson. (#2240) * adding a warning * adding warning regarding SIMDJSON_BUILD_STATIC_LIB * release candidate * pedantic viable size * Making tag_invoke a feeder instead of a producer * adding missing undef silencer (#2253) * Ignore pragma once when amalgamating source files (#2248) With gcc it causes an error in `simdjson.cpp`: ``` simdjson.cpp:548:9: warning: #pragma once in main file 548 | #pragma once | ^~~~ ``` It had previously been commented out in: 6ef555e However, this was lost in an upgrade: 2a4ff73 * Update CI (#2254) * adding missing undef silencer * Updating CI * more fixes * fix * big endian fix * Moving to the new tag_invoke signature * Fix nlohmann ambiguity on C++23-enabled clang * Revert "Merge branch 'master' of https://github.com/simdjson/simdjson into builder_development_branch_extra" This reverts commit 3eeecba, reversing changes made to 6858b20. --------- Co-authored-by: Daniel Lemire <daniel@lemire.me> Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com> Co-authored-by: John Keiser <john@johnkeiser.com> Co-authored-by: Tan Li Boon <undisputed-seraphim@users.noreply.github.com> Co-authored-by: tobil4sk <tobil4sk@outlook.com> * update CI on the builder_development_branch (no code change) (#2262) * typo * General madness simpler, no simpler!!! (#2267) * Minimal tag_invokes for STL types * simpler madness * adding a comment * missing file * minor tweaks to style * fixing incorrect max/min usage * updating single * simplify * validating the idea * putting back the concept * moving the include * guarding * Cheap General Madness (#2268) * Some General Concepts and their deserializations * Resolving ambiguity * Add missing #include * C++20 custom deserializer: better documentation (#2269) * mostly a documentation update. * missing cpp * [no-ci] fix comment * various minor fixes --------- Co-authored-by: Daniel Lemire <dlemire@lemire.me> --------- Co-authored-by: M. Bahoosh <12122474+the-moisrex@users.noreply.github.com> Co-authored-by: Daniel Lemire <dlemire@lemire.me> Co-authored-by: M. Bahoosh <moisrex@gmail.com> * minor update * More documentation regarding builder (#2270) * minor update * more improvment to our documentation (builder branch) * putting back missing functions * merge candidate --------- Co-authored-by: M. Bahoosh <moisrex@gmail.com> Co-authored-by: Daniel Lemire <dlemire@lemire.me> Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com> Co-authored-by: John Keiser <john@johnkeiser.com> Co-authored-by: Tan Li Boon <undisputed-seraphim@users.noreply.github.com> Co-authored-by: tobil4sk <tobil4sk@outlook.com> Co-authored-by: M. Bahoosh <12122474+the-moisrex@users.noreply.github.com>

lemire and others added 18 commits August 7, 2024 20:15

fix: add tests related to issue 2227 (simdjson#2229)

d506af0

* fix: add tests related to issue 2227 * avoiding name clash * pedantic fix * deprecate rvalue get on document * selectively deprecating

Fix ndjson spec link (simdjson#2234)

fc61d7c

* fix ndjson spec link The link in the readme of parse_many links to a casino spam site * fix link

[no-ci] Update README.md

7a9ff93

Make simdjson compile again

ef563a4

Enable SIMDJSON_SINGLEHEADER=OFF in VS Code

4c1b0a4

With singleheader on, clangd can't find the right include files.

Merge pull request simdjson#2237 from simdjson/jkeiser/simdjson-vscode

615218a

Reduce errors in vscode

Add missing include directives to static build targets of simdjson. (s…

0679c24

…imdjson#2240)

adding a warning

dd92151

adding warning regarding SIMDJSON_BUILD_STATIC_LIB

6b9117c

Merge branch 'master' of github.com:simdjson/simdjson

0ac0a80

release candidate

e341c8b

pedantic viable size

70a68da

Making tag_invoke a feeder instead of a producer

1691351

adding missing undef silencer (simdjson#2253)

025a443

Update CI (simdjson#2254)

e00cc8c

* adding missing undef silencer * Updating CI * more fixes * fix * big endian fix

Moving to the new tag_invoke signature

d6867ef

Fix nlohmann ambiguity on C++23-enabled clang

6858b20

the-moisrex mentioned this pull request Sep 18, 2024

object::find_all: find multiple keys in one go #2235

Closed

Merge branch 'master' of https://github.com/simdjson/simdjson into bu…

3eeecba

…ilder_development_branch_extra `document::get()&&` is now deprecated

lemire requested changes Sep 21, 2024

View reviewed changes

Revert "Merge branch 'master' of https://github.com/simdjson/simdjson …

463f5aa

…into builder_development_branch_extra" This reverts commit 3eeecba, reversing changes made to 6858b20.

lemire approved these changes Sep 21, 2024

View reviewed changes

lemire merged commit 49b9860 into simdjson:builder_development_branch Sep 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making `tag_invoke` a "put" as opposed to a "get"#2256

Making `tag_invoke` a "put" as opposed to a "get"#2256
lemire merged 20 commits intosimdjson:builder_development_branchfrom
the-moisrex:builder_development_branch_extra

the-moisrex commented Sep 18, 2024

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

the-moisrex commented Sep 18, 2024

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

lemire commented Sep 21, 2024 •

edited

Loading

Uh oh!

lemire left a comment

Uh oh!

lemire commented Sep 21, 2024

Uh oh!

lemire commented Sep 21, 2024

Uh oh!

the-moisrex commented Sep 21, 2024

Uh oh!

lemire left a comment

Uh oh!

lemire commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

the-moisrex commented Sep 18, 2024

Changes

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

the-moisrex commented Sep 18, 2024

Uh oh!

lemire commented Sep 18, 2024

Uh oh!

lemire commented Sep 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lemire left a comment

Choose a reason for hiding this comment

Uh oh!

lemire commented Sep 21, 2024

Uh oh!

lemire commented Sep 21, 2024

Uh oh!

the-moisrex commented Sep 21, 2024

Uh oh!

lemire left a comment

Choose a reason for hiding this comment

Uh oh!

lemire commented Sep 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

lemire commented Sep 21, 2024 •

edited

Loading