Skip to content

Conversation

@firewave
Copy link
Collaborator

@firewave firewave commented Feb 4, 2022

Currently not used in CI.

Have a follow-up PR which switches the most obvious case to SmallVector.

@danmar
Copy link
Owner

danmar commented Feb 12, 2022

approach looks ok to me.

@firewave
Copy link
Collaborator Author

I am still testing the follow-up PR which brings the improvements and I might need to adjust the alias still.

@firewave firewave marked this pull request as ready for review February 13, 2022 23:20
@firewave firewave marked this pull request as draft February 13, 2022 23:34
@firewave firewave marked this pull request as ready for review February 14, 2022 00:06

#include <cstddef>

static constexpr std::size_t DefaultSmallVectorSize = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldnt we want to default to be something like 4?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will be adjusted when the actual usage is added. Seems we will end up with 5 for that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to set this to 5?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I said I will do this in the follow-up(s) (one will drop shortly after this is in, the other in the following days). Currently nobody is using it and I want those changes tied together.

#include <vector>

template<typename T, std::size_t N = DefaultSmallVectorSize>
using SmallVector = std::vector<T>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a custom allocator so we can catch mismatches even when we aren't using boost small_vector:

template<class T, std::size_t N>
struct TaggedAllocator : std::allocator<T>
{
    template<class... Ts>
    TaggedAllocator(Ts&&... ts)
    : std::allocator<T>(std::forward<Ts>(ts)...)
    {}
};

template<typename T, std::size_t N = DefaultSmallVectorSize>
using SmallVector = std::vector<T, TaggedAllocator<T, N>>;

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright. Let me test that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine. Makes it also slightly faster with GCC.

@firewave firewave force-pushed the boost branch 2 times, most recently from 7f47ec2 to 5abb905 Compare February 14, 2022 12:48
@firewave
Copy link
Collaborator Author

@pfultz2 Does this look okay now?

Copy link
Owner

@danmar danmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if @pfultz2 is happy I am happy

@pfultz2
Copy link
Contributor

pfultz2 commented Mar 20, 2022

Yea LGTM

@firewave firewave merged commit 9d4fb16 into danmar:main Mar 20, 2022
@firewave firewave deleted the boost branch March 20, 2022 09:13
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.

3 participants