Init InplaceVector #1

Hels15 · 2024-08-12T12:02:39Z

Added basic container
Basic test cases(not complete, quite lacking)

- Got rid of relative paths

JeffGarland · 2024-08-19T13:55:40Z

Include/Beman/InplaceVector/InplaceVector.hpp

+    this->change_size(diff);
+  }; // freestanding-deleted
+  constexpr void assign(std::initializer_list<T> il) {
+    if constexpr (Capacity == 0) {


Is this correct for an empty initializer list? It would prevent the construction of a zero length inplace vector which I think we want to allow?

edit: Also is the correct response actually to throw bad_alloc? because of this: https://eel.is/c++draft/inplace.vector.overview#5

edit2: Since the requirements for initializer list aren't overridden the general sequence container requirements are going to apply - so here: https://eel.is/c++draft/sequence.reqmts#14

Since it's the equivalent of calling with an empty range I think it's a noop. This might be a wording corner case that isn't clear in the standard. I might post on list to get a clarification.

I had the same thought as I was reviewing this. Note that this question also applies to the assert in the other assign calls as well.

The correct behavior is to throw std::bad_alloc if the arguments to the assign call have a size greater than the inplace_vector's capacity. This is identical to what would happen if std::vector were assigned to a range sized greater than the available memory on the machine.

If I understand it right, the if constexpr (Capacity == 0) assert should be removed from all the assign functions, and should be replaced with std::bad_alloc if the arguments passed in for the assign call have greater size than the capacity? @camio @JeffGarland

Yes, exactly.

JeffGarland · 2024-08-19T13:56:49Z

Include/Beman/InplaceVector/InplaceVector.hpp

+    }
+    const auto diff = static_cast<std::ptrdiff_t>(il.size() - this->size());
+    if (diff < 0) {
+      const iterator new_end = std::copy(il.begin(), il.end(), this->begin());


Since we're in c++20 mode here std::ranges::copy is available.

Should all the copies use std.:ranges::copy? I guess so.

If it doesn't significantly hurt compilation performance or readability, I don't see why not.

JeffGarland · 2024-08-19T15:16:12Z

Include/Beman/InplaceVector/InplaceVector.hpp

+  }; // freestanding-deleted
+
+  // [containers.sequences.inplace.vector.access], element access
+  constexpr reference at(const size_type count) {


https://eel.is/c++draft/inplace.vector.overview standard doesn't have const in the signature -- that might fall into implementer freedom though...

constexpr reference at(size_type n); // freestanding-deleted constexpr const_reference at(size_type n) const; // freestanding-deleted

JeffGarland · 2024-08-19T15:22:02Z

Include/Beman/InplaceVector/InplaceVector.hpp

+    return emplace(position, std::move(x));
+  }; // freestanding-deleted
+  constexpr iterator insert(const_iterator position, size_type n, const T &x) {
+    const iterator pos = (iterator)position;


const_cast to be more specific?

edit: actually, is it actually needed at all -- I think it will convert

yes, I think both const iterator pos and position have the same type in this case.

JeffGarland · 2024-08-19T15:27:09Z

Include/Beman/InplaceVector/InplaceVector.hpp

+  } // freestanding-deleted
+  constexpr iterator insert(const_iterator position,
+                            std::initializer_list<T> il) {
+    const iterator pos = (iterator)position;


See line 679 -- if that one converts why is the cast needed?

src/Beman/InplaceVector/tests/InplaceVector.test.cpp

steve-downey · 2024-08-20T00:55:15Z

On Mon, Aug 19, 2024, 11:16 Jeff Garland ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Include/Beman/InplaceVector/InplaceVector.hpp <#1 (comment)> : > + return; + } + const auto diff = static_cast<std::ptrdiff_t>(il.size() - this->size()); + if (diff < 0) { + const iterator new_end = std::copy(il.begin(), il.end(), this->begin()); + std::destroy(new_end, this->end); + } else { + std::copy(il.begin(), il.begin() + this->size(), this->begin()); + base::uninitialized_copy(il.begin() + this->size(), il.end(), + this->end()); + } + this->change_size(diff); + }; // freestanding-deleted + + // [containers.sequences.inplace.vector.access], element access + constexpr reference at(const size_type count) { https://eel.is/c++draft/inplace.vector.overview standard doesn't have const in the signature -- that might fall into implementer freedom though...

const on size_t as a parameter is a no-op though? It does mean that the implementation can't modify their copy of the parameter, but the caller doesn't care. (Except on old Solaris compilers that mangled things incorrectly.)

…

constexpr reference at(size_type n); // freestanding-deleted constexpr const_reference at(size_type n) const; // freestanding-deleted — Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5TD3P43LV5SGFA6F5TZSID5BAVCNFSM6AAAAABMMAY5G6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDENBVHAZDSOJZGM> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

inbal2l · 2024-08-20T11:29:15Z

src/Beman/InplaceVector/inplacevector.cpp

@@ -1,3 +1 @@
-int main() {


Is the case inconsistency with the .hpp file on the filename intentional?

steve-downey

A few form issues. I'll look at substance soon too.

We still need a testing strategy for catching Mandates (static_assert) errors.

The constify function in Optional26 seems to be useful for constexpr testing, it's identity in a consteval context. Consteval functions that return something testable to EXPECT were good for, for example, testing constexpr emplace for optional.

steve-downey · 2024-08-20T13:03:53Z

src/Beman/InplaceVector/CMakeLists.txt

 target_include_directories(
        InplaceVector
        PUBLIC
-            $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../include>
-            $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_LOWER_PROJECT_NAME}>
+        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../Include>


I don't particularly like Capitalization in paths, but if that's consensus, OK, I can live with that.

Don't change FHS names. The unnecessary include directory in the source repo is spelled "include" and the files are installed into PREFIX/include.

steve-downey · 2024-08-20T13:05:08Z

src/Beman/InplaceVector/CMakeLists.txt

 install(
        DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_LOWER_PROJECT_NAME}
        FILES_MATCHING
-        PATTERN "*.hpp")
+        PATTERN "*.hpp"
+)



Globbing is still bad in Cmake, even if it's a little better. List the files.

I believe the best practice is to use FILE_SET as part of the target in CMake so the exact files get installed.

steve-downey · 2024-08-20T13:09:11Z

src/Beman/InplaceVector/tests/InplaceVector.test.cpp

@@ -0,0 +1,88 @@
+#include <Beman/InplaceVector/InplaceVector.hpp>


If this is <inplace_vector> it would make sense to spell it that way, at least inplace_vector.hpp, which exports Beman::InplaceVector::inplace_vector<>

Another thread on this elsewhere. I have to say i dislike the camel case in directory/files very much -- every press of the shift key slows things down.

steve-downey · 2024-08-20T13:11:22Z

src/Beman/InplaceVector/tests/InplaceVector.test.cpp

@@ -0,0 +1,88 @@
+#include <Beman/InplaceVector/InplaceVector.hpp>
+
+using namespace beman::InplaceVector;


s/beman/Beman/

Be tediously consistent.

I'm not exactly sure what you mean here, where would I put "src/beman/Beman"?

@steve-downey is using UNIX hacker jargon. "s/beman/Beman" means "everywhere you see beman replace it with Beman". The origin of this jargon is the UNIX sed command.

And to be pedantic it should have been s/beman/Beman/g ;)

camio · 2024-08-20T19:42:18Z

CMakeLists.txt

@@ -10,6 +10,7 @@ project(
    DESCRIPTION "A dynamically-resizable vector with fixed capacity and embedded storage"
    LANGUAGES CXX
 )
+set(CMAKE_CXX_STANDARD 20)


To support the inclusion of this project within a containing CMake project, the compiler flags should be inherited. Did you require this line to compile?

The stlab library mentions CMAKE_CXX_STANDARD as sometimes necessary for a successful build, but it is used explicitly on the command line.

So should I remove it from the cmakelist file and explicitly provide it through the terminal? @camio

Include/Beman/InplaceVector/InplaceVector.hpp

camio · 2024-08-20T19:46:16Z

Include/Beman/InplaceVector/InplaceVector.hpp

@@ -0,0 +1,750 @@
+// A ContiguousContainer is a Container that stores objects in contiguous memory
+// locations.
+


Did you consider naming this header inplace_vector.hpp for consistency with the paper which uses #include <inplace_vector>.

This might be a policy question for Beman as well -- do we want to distinguish from the standard or do we want minimal translation from beman to std

camio · 2024-08-20T19:48:10Z

Include/Beman/InplaceVector/InplaceVector.hpp

@@ -0,0 +1,750 @@
+// A ContiguousContainer is a Container that stores objects in contiguous memory
+// locations.
+


See [FILE.LICENSE_ID]. This also applies to other files here.

camio · 2024-08-20T19:53:23Z

Include/Beman/InplaceVector/InplaceVector.hpp

+#include <cassert>
+#include <compare>
+#include <concepts>
+#include <iostream>


Did you consider removing this header? It doesn't seem to be used. The same applies to #include <vector> below.

good point -- iostream is a bad thing to drag in bc of static objects cout, cerr etc. They would prevent use in an embedded context.

camio · 2024-08-20T21:36:09Z

Include/Beman/InplaceVector/InplaceVector.hpp

+      emplace_back(std::forward<decltype(value)>(value));
+    }
+  }
+#else


Suggested change

#else

I'm not sure what you are suggesting here, and GitHub doesn't seem to show much.

camio · 2024-08-20T22:54:28Z

Include/Beman/InplaceVector/InplaceVector.hpp

+    this->change_size(diff);
+  }; // freestanding-deleted
+  constexpr void assign(std::initializer_list<T> il) {
+    if constexpr (Capacity == 0) {


I had the same thought as I was reviewing this. Note that this question also applies to the assert in the other assign calls as well.

The correct behavior is to throw std::bad_alloc if the arguments to the assign call have a size greater than the inplace_vector's capacity. This is identical to what would happen if std::vector were assigned to a range sized greater than the available memory on the machine.

camio · 2024-08-20T22:59:39Z

src/Beman/InplaceVector/CMakeLists.txt

@@ -2,29 +2,40 @@
 # src/Beman/InplaceVector/CMakeLists.txt -*-makefile-*-
 # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 # cmake-format: on
-
+# Add the InplaceVector library
 add_library(InplaceVector STATIC inplacevector.cpp)


The discussion on what the target name should be is still ongoing. I believe it should at least have the Beman prefix so probably Beman.InplaceVector should be the library name.

camio · 2024-08-20T23:00:38Z

src/Beman/InplaceVector/CMakeLists.txt

 install(
        DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
        DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_LOWER_PROJECT_NAME}
        FILES_MATCHING
-        PATTERN "*.hpp")
+        PATTERN "*.hpp"
+)



I believe the best practice is to use FILE_SET as part of the target in CMake so the exact files get installed.

camio · 2024-08-20T23:01:41Z

src/Beman/InplaceVector/tests/CMakeLists.txt

@@ -5,7 +5,7 @@

 # Tests
 add_executable(
-        InplaceVectorTest.test)
+        InplaceVectorTest.test InplaceVector.test.cpp)


Suggested change

InplaceVectorTest.test InplaceVector.test.cpp)

Beman.InplaceVectorTest.test InplaceVector.test.cpp)

All targets need a Beman prefix in case this is incorporated in a larger CMake project. (targets are in a global namespace).

Co-authored-by: David Sankel <[email protected]>

Hels15 · 2024-08-21T09:48:12Z

I will need some time to resolve all these issues, thank you everyone for reviewing it.

`inplace_vector_size_type` to `inplace_vector_internal_size_type`.

…vector.cpp

# Conflicts: # Include/Beman/InplaceVector/inplace_vector.hpp

Hels15 · 2024-08-25T14:08:52Z

@steve-downey @camio @JeffGarland @inbal2l, Please provide me a new review/resolve them if I fixed the problem in the latest commit.

steve-downey · 2024-08-25T19:03:32Z

Will look

JeffGarland · 2024-08-25T23:29:07Z

Include/Beman/InplaceVector/inplace_vector.hpp

 template <typename T>
 constexpr bool container_compatible_range =
    detail::container_compatible_range_impl<T>;

 template <bool Condition, typename TrueType, typename FalseType>
 using If = typename std::conditional<Condition, TrueType, FalseType>::type;

+// The internal size type can be smaller than std::size_t when capacity allows
+// for it.


JeffGarland · 2024-08-25T23:41:40Z

Include/Beman/InplaceVector/inplace_vector.hpp

@@ -191,13 +192,13 @@ struct inplace_vector_base : public inplace_vector_destruct_base<T, Capacity> {
  template <typename Iter>
  static constexpr void uninitialized_copy(Iter first, Iter last,
                                           iterator dest) noexcept {


I think we need conditional noexcept here

So here I need to consider the cases where it is guaranteed that the function doesn't throw any exception. Since it does copying - I think a traditional

noexcept(std::is_nothrow_copy_constructible_v<typename std::iterator_traits<Iter>::value_type>)

should suffice.
@JeffGarland

That looks correct to me.

Include/Beman/InplaceVector/inplace_vector.hpp

JeffGarland · 2024-08-25T23:57:18Z

I've finished pass 2. This is looking pretty good - thx @Hels15!

Hels15 · 2024-09-05T18:21:31Z

What is the current state of this PR now? @JeffGarland @steve-downey

JeffGarland · 2024-09-16T21:42:35Z

Include/Beman/InplaceVector/inplace_vector.hpp

+  using size_type = std::size_t;
+  using internal_size_type = inplace_vector_internal_size_type<Capacity>;
+
+  alignas(T) unsigned char elems[Capacity * sizeof(T)] = {};


something I notice on the second pass -- In this paper at least the recommendation is to use std::byte with alignas -- I'm guessing unsigned char is the same, but I'm not alignment expert. I don't want to hold the PR for this, but curious if the better practice now is byte.

https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1413r3.pdf

JeffGarland · 2024-09-16T21:45:13Z

Include/Beman/InplaceVector/inplace_vector.hpp

+  inplace_vector_base(const inplace_vector_base &other) noexcept(
+      std::is_nothrow_copy_constructible_v<T>)
+      : inplace_vector_destruct_base<T, Capacity>(other.size) {
+    std::ranges::copy(other.begin(), other.end(), begin());


Does this not work?

std::ranges::copy(other, begin());

JeffGarland · 2024-09-16T21:46:56Z

Include/Beman/InplaceVector/inplace_vector.hpp

+      // copy other vector into the current vector until it runs ouf of size
+      std::ranges::copy(other.begin(), other.begin() + size(), begin());
+      // copy the other half after the end of the current vector
+      std::ranges::copy(other.begin() + size(), other.end(), end());


The 2 copies above need the 2 iterator overload even with ranges::copy since they are partial -- so in this case it makes sense.

JeffGarland · 2024-09-16T21:56:27Z

Include/Beman/InplaceVector/inplace_vector.hpp

+  constexpr void append_range(R &&rg) {
+    auto first = rg.begin();
+    auto last = rg.end();
+    for (; first != last; ++first) {


should we consider re-writing with range-for loop. I think that would look something like:

for ( auto& current : rg ) { emplace_back(current); }

steve-downey · 2024-09-16T21:57:17Z

std::byte is the current model for the smallest addressable piece of memory, so that makes sense, I suppose. It doesn't have all of the integral things that a char has, but still allows aliasing, by fiat.

…

On Mon, Sep 16, 2024, 17:42 Jeff Garland ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In Include/Beman/InplaceVector/inplace_vector.hpp <#1 (comment)> : > +// The internal size type can be smaller than std::size_t when capacity allows +// for it. +template <std::size_t Capacity> +using inplace_vector_internal_size_type = + If<Capacity <= std::numeric_limits<uint8_t>::max(), uint8_t, + If<Capacity <= std::numeric_limits<uint16_t>::max(), uint16_t, + If<Capacity <= std::numeric_limits<uint32_t>::max(), uint32_t, + uint64_t>>>; + +// Base class for inplace_vector +template <typename T, std::size_t Capacity> +struct inplace_vector_destruct_base { + using size_type = std::size_t; + using internal_size_type = inplace_vector_internal_size_type<Capacity>; + + alignas(T) unsigned char elems[Capacity * sizeof(T)] = {}; something I notice on the second pass -- In this paper at least the recommendation is to use std::byte with alignas -- I'm guessing unsigned char is the same, but I'm not alignment expert. I don't want to hold the PR for this, but curious if the better practice now is byte. https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p1413r3.pdf — Reply to this email directly, view it on GitHub <#1 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVNZ5RDVJCGRE7H3AWJW4TZW5GGBAVCNFSM6AAAAABMMAY5G6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGMBXHA4TOMZWGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

JeffGarland · 2024-09-16T22:04:43Z

Include/Beman/InplaceVector/inplace_vector.hpp

+      emplace_back(*first);
+    }
+  }; // freestanding-deleted
+  constexpr void pop_back() {


I'm also wondering about conditional noexcept here -- it's not noexcept in any form, in the standard text, but certainly could be if T is no throw destructible. Also it's not marked as freestanding deleted -- which I suppose is because no T on freestanding is allowed to throw? Worrying we might have some spec bugs here.

Re: freestanding
The design in the standard is to avoid originating exceptions. If a user type throws first, then that isn't an issue. This comes to the forefront in std::visit, as it can throw if a variant is in the valueless state. You can only get on the valueless state when an exception is thrown though.

As for the noexceptness, I think it isn't currently noexcept because there's a precondition on the container being non-empty. I thought we generally assumed stuff was noexcept destructible. I think this could be marked unconditionally noexcept.

init2

ef3c270

steve-downey self-requested a review August 12, 2024 12:54

- Extended cmake with tests

1f6e807

- Got rid of relative paths

Hels15 mentioned this pull request Aug 16, 2024

Implement Beman.InplaceVector beman-project/beman#37

Closed

JeffGarland reviewed Aug 19, 2024

View reviewed changes

src/Beman/InplaceVector/tests/InplaceVector.test.cpp Show resolved Hide resolved

inbal2l reviewed Aug 20, 2024

View reviewed changes

steve-downey reviewed Aug 20, 2024

View reviewed changes

camio reviewed Aug 20, 2024

View reviewed changes

Hels15 and others added 2 commits August 21, 2024 10:19

Update Include/Beman/InplaceVector/InplaceVector.hpp

424bbc3

Co-authored-by: David Sankel <[email protected]>

proposed fixes

8a2fef4

Hels15 added 9 commits August 21, 2024 11:15

Minor cmake change(Beman prefix)

762e0a5

rename r_size_type to internal_size_type as well as

63ae0a9

`inplace_vector_size_type` to `inplace_vector_internal_size_type`.

Provide c++ version through terminal

01cc228

got rid of if capacity == 0

4148c2c

use ranges copy instead of plain copy

54ff78f

copy size in assignment, rename InplaceVector source file to inplace_…

ac95905

…vector.cpp

added more explanations for the std::range copy

79d600a

Todo was fixed

bf8abd9

Merge remote-tracking branch 'origin/master'

a9b93f4

# Conflicts: # Include/Beman/InplaceVector/inplace_vector.hpp

JeffGarland reviewed Aug 25, 2024

View reviewed changes

Include/Beman/InplaceVector/inplace_vector.hpp Show resolved Hide resolved

JeffGarland approved these changes Sep 12, 2024

View reviewed changes

camio merged commit 69deed7 into beman-project:main Sep 16, 2024

JeffGarland reviewed Sep 16, 2024

View reviewed changes

		@@ -0,0 +1,88 @@
		#include <Beman/InplaceVector/InplaceVector.hpp>

		@@ -0,0 +1,88 @@
		#include <Beman/InplaceVector/InplaceVector.hpp>

		using namespace beman::InplaceVector;

		@@ -0,0 +1,750 @@
		// A ContiguousContainer is a Container that stores objects in contiguous memory
		// locations.

	InplaceVectorTest.test InplaceVector.test.cpp)
	Beman.InplaceVectorTest.test InplaceVector.test.cpp)

Init InplaceVector #1

Init InplaceVector #1

Conversation

Hels15 commented Aug 12, 2024

JeffGarland Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffGarland Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffGarland Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

steve-downey commented Aug 20, 2024 via email

Choose a reason for hiding this comment

steve-downey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hels15 Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hels15 commented Aug 21, 2024

Hels15 commented Aug 25, 2024

steve-downey commented Aug 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffGarland commented Aug 25, 2024

Hels15 commented Sep 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffGarland Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steve-downey commented Sep 16, 2024 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffGarland Aug 19, 2024 •

edited

Loading

JeffGarland Aug 19, 2024 •

edited

Loading

JeffGarland Aug 19, 2024 •

edited

Loading

Hels15 Aug 21, 2024 •

edited

Loading

JeffGarland Sep 16, 2024 •

edited

Loading