initial version of InlineVector #3

novakovicdj · 2024-12-02T09:46:55Z

CAHEK7

Also could you add modifiers like

clear
push_back
emplace_back
pop_back
resize

Please refer to the std::vector implementation.

The rest of modifiers like

insert 
emplace
erase
swap

probably are not so important, but much mor complex.

CAHEK7 · 2024-12-02T09:52:40Z

src/include/miopen/inline_vector.hpp

+
+    InlineVector(std::initializer_list<T> _data) : real_size(_data.size())
+    {
+        if(_data.size() > N)


You can actually use real_size here. It is already initialized.
(see the comment about real_size itself down below)

CAHEK7 · 2024-12-02T09:53:51Z

src/include/miopen/inline_vector.hpp

+    template <typename _InputIterator, typename = std::_RequireInputIter<_InputIterator>>
+    InlineVector(_InputIterator first, _InputIterator last)
+    {
+        if(std::distance(first, last) > N)
+        {
+            MIOPEN_THROW("Input data size is bigger than InlineVector's capacity");
+        }
+        std::copy(first, last, data.begin());
+        real_size = std::distance(first, last);
+    }


You can do exactly the same as in the InlineVector(std::initializer_list<T> _data) constructor, like : real_size(std::distance(...)).

CAHEK7 · 2024-12-02T09:59:30Z

src/include/miopen/inline_vector.hpp

+
+private:
+    std::array<T, N> data;
+    uint8_t real_size;


Even if real size (and N) can be relatively small, it's better keeping it as size_t. Most probably from 3 to 7 bytes will be consumed by padding.
Also it is better to keep some consistency between all the container implementations and it's safer to assign anything to that size_t real_size - assigning to int/uint8 may lead to some unexpected truncations.

CAHEK7 · 2024-12-02T10:00:56Z

src/include/miopen/inline_vector.hpp

+    T& operator[](std::size_t n) { return data[n]; }
+
+    const T& operator[](std::size_t n) const { return data[n]; }


I would add method .at() which checks the boundaries and throw.

CAHEK7 · 2024-12-02T10:07:05Z

src/include/miopen/inline_vector.hpp

+    bool empty() const { return real_size == 0; }
+
+    // Real size
+    uint8_t size() const { return real_size; }


Suggested change

uint8_t size() const { return real_size; }

uint8_t size() const noexcept { return real_size; }

The other tiny methods should have noexcept (like operators, iterators, etc, I guess almost all of the methods)

CAHEK7 · 2024-12-02T10:10:10Z

src/include/miopen/inline_vector.hpp

+    std::reverse_iterator<T*> rend() { return std::reverse_iterator<T*>(begin()); }
+
+    // Element access
+    T& operator[](std::size_t n) { return data[n]; }


Also front(), back() and data() methods should be added.

CAHEK7 · 2024-12-02T10:15:46Z

test/gtest/inline_vector_basic_ops.cpp

+    auto first_not_one_in_v2 =
+        std::find_if(in_v2.rbegin(), in_v2.rend(), [](int i) { return i != 1; });
+    auto first_note_one_v2 = std::find_if(v2.rbegin(), v2.rend(), [](int i) { return i != 1; });
+    EXPECT_EQ(*first_not_one_in_v2, *first_note_one_v2);


If something got broken, first_not_one_in_v2 can be end() and EXPECT_EQ has undefined behavior and does not check anything.

Probably std::distance check should be done prior to the value check.

CAHEK7 · 2024-12-02T10:18:04Z

test/gtest/inline_vector_basic_ops.cpp

+    int sum_in_v1 = std::accumulate(in_v1.begin(), in_v1.end(), 0);
+    int sum_v1    = std::accumulate(v1.begin(), v1.end(), 0);


I know that both vectors are 4, 2, 1, but accumulating size_t to int usually is not a good idea (it's not just int sum, the most important is the last parameter of std::accumulate).
Probably it's simpler to make both vectors int.

CAHEK7 · 2024-12-02T14:56:53Z

src/include/miopen/inline_vector.hpp

+    const T* cbegin() const noexcept { return _data.cbegin(); }
+
+    const T* cend() const noexcept { return (_data.cbegin() + real_size); }


it automatically resolves a const version

Suggested change

const T* cbegin() const noexcept { return _data.cbegin(); }

const T* cend() const noexcept { return (_data.cbegin() + real_size); }

const T* cbegin() const noexcept { return begin(); }

const T* cend() const noexcept { return end(); }

CAHEK7 · 2024-12-02T15:06:53Z

src/include/miopen/inline_vector.hpp

+        {
+            MIOPEN_THROW("Cannot get back element, InlineVector is empty");
+        }
+        return *(end() - 1);


Suggested change

return *(end() - 1);

return *std::prev(end());;

Usually front/back functions do not check ranges. but probably it's ok.

CAHEK7 · 2024-12-02T15:08:35Z

src/include/miopen/inline_vector.hpp

+    void resize(std::size_t n)
+    {
+        if(n > N)
+        {
+            MIOPEN_THROW("It is not possible to resize beyond capacity");
+        }
+
+        real_size = n;
+    }


That is actually

Suggested change

void resize(std::size_t n)

{

if(n > N)

{

MIOPEN_THROW("It is not possible to resize beyond capacity");

}

real_size = n;

}

void resize(std::size_t n)

{

resize(n, T{});

}

CAHEK7 · 2024-12-02T15:13:02Z

src/include/miopen/inline_vector.hpp

+    }
+
+    // Remove element from the back
+    void pop_back() noexcept { real_size = ((real_size - 1) >= 0) ? (real_size - 1) : 0; }


Suggested change

void pop_back() noexcept { real_size = ((real_size - 1) >= 0) ? (real_size - 1) : 0; }

void pop_back() noexcept { real_size = (real_size > 1) ? (real_size - 1) : 0; }

CAHEK7 · 2024-12-02T16:39:56Z

src/include/miopen/inline_vector.hpp

+    void pop_back() noexcept { real_size = ((real_size - 1) >= 0) ? (real_size - 1) : 0; }
+
+    // Clear
+    constexpr void clear() noexcept { real_size = 0; }


That shouldn't be a constexpr, unless we make some extra efforts to make the whole class constexpr friendly.
While capacity can be, should be and it is constexpr.

initial version of InlineVector

15bab39

CAHEK7 suggested changes Dec 2, 2024

View reviewed changes

added additional methods

87d4faf

CAHEK7 suggested changes Dec 2, 2024

View reviewed changes

CAHEK7 reviewed Dec 2, 2024

View reviewed changes

minor changes

740ba1a

novakovicdj closed this Dec 3, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

initial version of InlineVector #3

initial version of InlineVector #3

novakovicdj commented Dec 2, 2024

CAHEK7 left a comment

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

CAHEK7 Dec 2, 2024

		T& operator[](std::size_t n) { return data[n]; }

		const T& operator[](std::size_t n) const { return data[n]; }

	uint8_t size() const { return real_size; }
	uint8_t size() const noexcept { return real_size; }

		int sum_in_v1 = std::accumulate(in_v1.begin(), in_v1.end(), 0);
		int sum_v1 = std::accumulate(v1.begin(), v1.end(), 0);

		const T* cbegin() const noexcept { return _data.cbegin(); }

		const T* cend() const noexcept { return (_data.cbegin() + real_size); }

	void pop_back() noexcept { real_size = ((real_size - 1) >= 0) ? (real_size - 1) : 0; }
	void pop_back() noexcept { real_size = (real_size > 1) ? (real_size - 1) : 0; }

initial version of InlineVector #3

initial version of InlineVector #3

Conversation

novakovicdj commented Dec 2, 2024

CAHEK7 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment