Skip to content

added SmallVector alias with conditional boost::container version#3799

Merged
firewave merged 3 commits intocppcheck-opensource:mainfrom
firewave:boost
Mar 20, 2022
Merged

added SmallVector alias with conditional boost::container version#3799
firewave merged 3 commits intocppcheck-opensource:mainfrom
firewave:boost

Conversation

@firewave
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

danmar commented Feb 12, 2022

approach looks ok to me.

@firewave
Copy link
Copy Markdown
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
Comment thread lib/smallvector.h

#include <cstddef>

static constexpr std::size_t DefaultSmallVectorSize = 0;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

Comment thread lib/smallvector.h Outdated
#include <vector>

template<typename T, std::size_t N = DefaultSmallVectorSize>
using SmallVector = std::vector<T>;
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Collaborator Author

@pfultz2 Does this look okay now?

Copy link
Copy Markdown
Collaborator

@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
Copy Markdown
Contributor

pfultz2 commented Mar 20, 2022

Yea LGTM

@firewave firewave merged commit 9d4fb16 into cppcheck-opensource: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