Skip to content

Add support for inplace_vector #1729

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

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

20162026
Copy link
Contributor

@20162026 20162026 commented May 5, 2025

@20162026
Copy link
Contributor Author

20162026 commented May 5, 2025

It works, but I'm not sure if it is worthy to be merged becouse:

  1. only json read is implemented ( beve dose not use emplace_back and would require more modifications, didn't check others)
  2. serializing into inplace_vector<char, N> without exceptions is tricky becouse of multiple resize calls
  3. beman::inplace_vector is not fully implemented (e.g. <=> is broken, no proper constexpr handling, init list only works with exceptions enabled, build fails with clang fnoexcept)

@stephenberry
Copy link
Owner

Thanks for making this merge request! I do like the idea of inplace_vector support and this gives a starting point for experimenting with that support. I'll look into them when I get some time. I wouldn't mind merging in a solution that is 90% there and improving it over time.

@20162026
Copy link
Contributor Author

20162026 commented May 9, 2025

I will try to bring beman.inplace_vector into a somewhat usable state before continuing with this PR.

If anyone knows of a good inplace_vector implementation that doesn’t require a full custom STL, please let me know :)

@stephenberry
Copy link
Owner

@20162026, I walked AI through generating an inplace_vector implementation based only the STL and using C++23. I've added it to Glaze: glaze/containers/inplace_vector.hpp. Will need more testing, but I think this should be a good approach for implementing it until new get it in C++26. The goal will be to make it efficient while complying with the future standard.

@stephenberry
Copy link
Owner

stephenberry commented May 13, 2025

@20162026, I think glz::inplace_vector is looking pretty good now. It could probably be optimized in a few cases, but it should be high performance for trivial types. Let me know what you think. I added a bunch of unit tests to make sure things are working as expected.

@20162026
Copy link
Contributor Author

20162026 commented May 13, 2025

@stephenberry would you be interested in supporting freestanding inplace_vector in glaze, that would =delete all of the potentaly throwing functions? According to spec all throwing functions are marked with 'freestanding-deleted` which means its up to implementer if they want to leave them or remove.

deleting throwing/aborting functions is extremely convenient for embedded, as you get compiler warning instead of runtime error when trying to use potentially throwing functions. But it would require at least some minor concept changes as it will no longer be direct vector replacement.

also as a sidenot, would you like me to add pre-commit for formatting?

@stephenberry
Copy link
Owner

@20162026, yes I think 'freestanding-deleted' makes sense. Go ahead.

And, per your side note about formatting, Glaze is actually setup to use clang-format (just upgraded to v20) automatically when the code gets merged, so don't worry about formatting this merge request.

@20162026
Copy link
Contributor Author

20162026 commented May 29, 2025

@stephenberry is there actually performance gain in release build when using stuff like

if constexpr (std::is_trivially_copyable_v<T>) {
   std::memcpy(this->data_ptr() + storage_size(), &x, sizeof(T));
}
else {
   std::construct_at(this->data_ptr() + storage_size(), x);
}

or

if constexpr (std::is_trivially_copyable_v<T>) {
 return std::memcmp(lhs.data_ptr(), rhs.data_ptr(), lhs.storage_size() * sizeof(T)) == 0;
}
else {
 return std::equal(lhs.begin(), lhs.end(), rhs.begin());
}

I would expect compiler to be smart enough to optmize such case for trivial types?

@stephenberry
Copy link
Owner

@20162026, I fixed an issue with pop_back removing the wrong element, and I fixed some use of inplace_vector<T, 0>. After going through it again I think it is in a good place, and it really is mainly existing to test the concept usage in the rest of Glaze. Do you think it's in a good place to merge?

@20162026
Copy link
Contributor Author

Do you think it's in a good place to merge?

I will redo concepts for inplace_vector detection, as freestanding inplace_vector might not fullfill current vector_like concept.

@20162026
Copy link
Contributor Author

CI failed becouse security.ubuntu.com is dead xd, maybe try rerunning them when it is back up....

other than that, guess Im done with the changes for now

@20162026 20162026 marked this pull request as ready for review May 29, 2025 16:30
@stephenberry
Copy link
Owner

@20162026, CI passed.

@20162026
Copy link
Contributor Author

hi @stephenberry, don't want to rush you or sound rude, but what is the state of this PR? would you like to see some changes or is it ok as is?

@stephenberry
Copy link
Owner

I think it is in a good place. If you agree, I'll merge it in.

@20162026
Copy link
Contributor Author

ok, you can merge it

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.

2 participants