Common template for inline vector implementation. #245

lemur73 · 2021-05-17T17:50:20Z

Allows simplier using inline vectors of different sizes and types.

Signed-off-by: Ostap Slavking [email protected]
If you like my commits cheer me with some chia here:
xch16es77qggpwgzucqmmvhvgcueckmt9g3e7exa46vhmucz7z8j2a2spm7c7e

lgtm-com · 2021-05-17T18:09:04Z

This pull request fixes 1 alert when merging 51a5eee into 632c9d7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com · 2021-05-17T18:29:03Z

This pull request fixes 1 alert when merging 9fa9f9f into 632c9d7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

arvidn · 2021-05-18T07:16:21Z

this mostly looks like an improvement. However, when you make the element type a template argument I think you're going a bit too far. your InlineVector implementation does not support arbitrary element types. Fortunately, we don't need arbitrary types. I would suggest just leaving it hard coded as uint64_t.

One thing these classes need more urgently are tests though.

arvidn · 2021-05-18T07:17:07Z

src/bits.hpp

+template <class item_type, class size_type_arg, unsigned capacity>
+class InlineVector {
+  public:
+    typedef size_type_arg size_type;


the way this spells nowadays is:

Suggested change

typedef size_type_arg size_type;

using size_type = size_type_arg;

arvidn · 2021-05-18T07:18:13Z

src/bits.hpp


-    SmallVector& operator=(const SmallVector& other)
-    {
+    InlineVector& operator=(const InlineVector& other) {


while you're at it, this function should probably be ref-qualified.

Suggested change

InlineVector& operator=(const InlineVector& other) {

InlineVector& operator=(const InlineVector& other) & {

arvidn · 2021-05-18T07:18:57Z

src/bits.hpp

-    typedef uint32_t size_type;
-
-    ParkVector() noexcept { count_ = 0; }
+    InlineVector& operator=(const std::vector<item_type>& other) {


and so should this probably:

Suggested change

InlineVector& operator=(const std::vector<item_type>& other) {

InlineVector& operator=(const std::vector<item_type>& other) & {

One thing these classes need more urgently are tests though.

Updated with fixes from your comments and test.
PTAL

arvidn · 2021-05-18T07:20:06Z

src/bits.hpp

+
+// A stack vector of length 2048, having the functions of std::vector needed for Bits.
+// The max number of Bits that can be stored is 2048 * 64
+typedef InlineVector<uint64_t, uint16_t, 2048> ParkVector;


the modern way (and more readable in my opinion) is to use a using declaration.

Suggested change

typedef InlineVector<uint64_t, uint16_t, 2048> ParkVector;

using ParkVector = InlineVector<uint64_t, uint16_t, 2048>;

the modern way (and more readable in my opinion) is to use a using declaration.

Unfortunately, this doesn't work. Gives an error:

~/chiapos_up/src/bits.hpp:79: error: expected nested-name-specifier using InlineVector<uint64_t, uint8_t, 10> SmallVector; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

try this instead:

using ParkVector = InlineVector<uint64_t, uint16_t, 2048>;

That worked, thanks.
Updated and uploaded.

lemur73 · 2021-05-18T07:36:33Z

this mostly looks like an improvement. However, when you make the element type a template argument I think you're going a bit too far. your InlineVector implementation does not support arbitrary element types. Fortunately, we don't need arbitrary types. I would suggest just leaving it hard coded as uint64_t.

Well, I have some use of this template argument in my next commits. Just prefer to keep uploads smaller to make it easy to understand and review.

One thing these classes need more urgently are tests though.

I'll upload tests.

arvidn · 2021-05-18T08:05:31Z

src/bits.hpp

+    size_type max_size() const { return capacity; }
+
+  private:
+    item_type v_[capacity];
    size_type count_;


If you make this change, you don't have to define the constructor

Suggested change

size_type count_;

size_type count_ = 0;

lgtm-com · 2021-05-18T16:02:23Z

This pull request introduces 1 alert and fixes 1 when merging e94cf3f into 632c9d7 - view on LGTM.com

new alerts:

1 for Comparison of narrow type with wide type in loop condition

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com · 2021-05-18T17:11:08Z

This pull request fixes 1 alert when merging 8c67004 into 632c9d7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com · 2021-05-19T21:38:53Z

This pull request fixes 1 alert when merging c71e3eb into 632c9d7 - view on LGTM.com

fixed alerts:

1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

arvidn · 2021-05-20T06:49:07Z

src/bits.hpp

-    SmallVector() noexcept { count_ = 0; }
-
-    uint64_t& operator[](const uint16_t index) { return v_[index]; }
+template <class item_type, class size_type_arg, unsigned capacity>


Here's my argument against making item_type a template parameter:

As it stands in your patch, it's not correct. The template parameter cannot be an arbitrary type, since they're all constructed up-front they must be default constructable. In order to make this more correct, you would need something like:

static_assert(std::is_integral_t<item_type>, "InlineVector inly supports integral types");

Which brings me to my next point. We don't currently need this additional generality/complexity. I don't think it makes sense to make things more general/complex just for the sake of it (in fact, I would argue that keeping everything as specific as possible makes for easier-to-read programs).

Now, you say you'll need this template parameter in your next PR. So, let's punt this change until that PR.

The template parameter cannot be an arbitrary type, since they're all constructed up-front they must be default constructable.

Yeah, I figured that out already in my next patch. Let's see if you like it ;)
Added static assert for now.

arvidn · 2021-05-20T06:52:46Z

I think everything looks good apart from my last comment

Allows simplier using inline vectors of different sizes and types. Signed-off-by: Ostap Slavking <[email protected]> If you like my commits cheer me with some chia here: xch16es77qggpwgzucqmmvhvgcueckmt9g3e7exa46vhmucz7z8j2a2spm7c7e

github-actions · 2021-08-12T11:05:40Z

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

lemur73 force-pushed the main branch from 51a5eee to 9fa9f9f Compare May 17, 2021 18:14

arvidn reviewed May 18, 2021

View reviewed changes

lemur73 force-pushed the main branch from 9fa9f9f to e94cf3f Compare May 18, 2021 15:45

lemur73 force-pushed the main branch from e94cf3f to 8c67004 Compare May 18, 2021 16:57

lemur73 force-pushed the main branch from 8c67004 to c71e3eb Compare May 19, 2021 21:22

arvidn reviewed May 20, 2021

View reviewed changes

lemur73 force-pushed the main branch from c71e3eb to 9da1bb4 Compare May 20, 2021 17:38

Common template for inline vector implementation.

3e1545b

Allows simplier using inline vectors of different sizes and types. Signed-off-by: Ostap Slavking <[email protected]> If you like my commits cheer me with some chia here: xch16es77qggpwgzucqmmvhvgcueckmt9g3e7exa46vhmucz7z8j2a2spm7c7e

lemur73 force-pushed the main branch from 9da1bb4 to 3e1545b Compare May 20, 2021 17:41

github-actions bot added the stale-pr label Aug 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common template for inline vector implementation. #245

Common template for inline vector implementation. #245

lemur73 commented May 17, 2021

lgtm-com bot commented May 17, 2021

lgtm-com bot commented May 17, 2021

arvidn commented May 18, 2021

arvidn May 18, 2021

arvidn May 18, 2021

arvidn May 18, 2021

lemur73 May 18, 2021

arvidn May 18, 2021

lemur73 May 18, 2021

arvidn May 18, 2021

lemur73 May 19, 2021

lemur73 commented May 18, 2021

arvidn May 18, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 19, 2021

arvidn May 20, 2021

lemur73 May 20, 2021

arvidn commented May 20, 2021

github-actions bot commented Aug 12, 2021

	typedef size_type_arg size_type;
	using size_type = size_type_arg;

	InlineVector& operator=(const InlineVector& other) {
	InlineVector& operator=(const InlineVector& other) & {

	InlineVector& operator=(const std::vector<item_type>& other) {
	InlineVector& operator=(const std::vector<item_type>& other) & {

	typedef InlineVector<uint64_t, uint16_t, 2048> ParkVector;
	using ParkVector = InlineVector<uint64_t, uint16_t, 2048>;

Common template for inline vector implementation. #245

Are you sure you want to change the base?

Common template for inline vector implementation. #245

Conversation

lemur73 commented May 17, 2021

lgtm-com bot commented May 17, 2021

lgtm-com bot commented May 17, 2021

arvidn commented May 18, 2021

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

lemur73 commented May 18, 2021

Choose a reason for hiding this comment

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 18, 2021

lgtm-com bot commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arvidn commented May 20, 2021

github-actions bot commented Aug 12, 2021