Skip to content

Adding VariableLengthArray and PMR namespace - #28

Merged
thirtytwobits merged 10 commits into
mainfrom
develop/vla
Jun 7, 2023
Merged

thirtytwobits merged 10 commits into
mainfrom
develop/vla

Conversation

@thirtytwobits

Copy link
Copy Markdown
Member

Providing a polyfill of much of the pmr namespace for C++14.

Adding VariableLengthArray as an allocator-aware vector. CETL's VLA differs from vector in three respects:

  1. it supports the CETL-specific reallocation extension to PMR::memory_resource
  2. it will tightly fit into a maximum size when elements are pushed without calling reserve first.
  3. it avoids undefined behaviour when exceptions are disabled.

An unintended benefit: this works with C++17/AppleClang which does not support PMR.

Reviewer Notes

As always, feel free to skip the vscode/cmake stuff. There was a major refactor of this as I learned more about cmake and I enabled sharing modules with dependent projects (i.e. libcyphal).

This is a huge change. Sorry. Circumstances prevented me from getting PMR in before VLA. If the review is too daunting I can split it up into three stages but it would take some time to get each building independently. One way to make this easier is to review everything under the PMR namespace first and ignore VLA to start with then come back to the VLA.

There are known missing comments, unittests, and CI fix-ups that I'll be working on in parallel.

Providing a polyfill of much of the pmr namespace for C++14.

Adding VariableLengthArray as an allocator-aware vector. CETL's VLA differs from vector in three respects:
1. it supports the CETL-specific reallocation extension to PMR::memory_resource
2. it will tightly fit into a maximum size when elements are pushed without calling reserve first.
3. it avoids undefined behaviour when exceptions are disabled.

An unintended benefit: this works with C++17/AppleClang which does not support PMR.
Comment thread include/cetl/variable_length_array.hpp Outdated
Comment thread .vscode/open_docs_mac.sh Outdated
@pavel-kirienko

Copy link
Copy Markdown
Member

Is there some place on the web where the docs could be deployed for review?

@lydia-at-amazon lydia-at-amazon changed the title Adding VariableLengthArray and PMR namesapce Adding VariableLengthArray and PMR namespace May 31, 2023
@thirtytwobits

Copy link
Copy Markdown
Member Author

Is there some place on the web where the docs could be deployed for review?

I fixed the CI enough that the latest run uploaded an archive of the docs.

Also: The CI revealed that std::vector can refuse to shrink_to_fit when exceptions are disabled. I'll need to fixup the tests to account for this.

@pavel-kirienko pavel-kirienko left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure I understand the purpose of clang-format-check.py when the same could be achieved in fewer lines of code via a custom cmake target (unless I'm missing something), but I presume it is comparatively unimportant. I mostly skipped all of the copious scaffolding and focused on the docs & code.

Comment thread include/cetl/pf17/memory_resource.hpp
Comment thread include/cetl/pmr/memory.hpp
@thirtytwobits

Copy link
Copy Markdown
Member Author

clang-format-check.py when the same could be achieved in fewer lines of code via a custom cmake target

Do tell! I'd love to get rid of this if I could but I didn't want to try to parse xml in cmake.

@thirtytwobits

Copy link
Copy Markdown
Member Author

would strongly prefer to have VariableLengthArray::at(std::size_t) defined similarly to...

Sorry, but I'm not going to put a production abort() into the code. You can implement an external helper:

template<typename VLAType>
const typename VLAType::value_type& at(const VLAType& container, std::size_t pos)
{
    if (pos >= container.size())
    {
#if __cpp_exceptions
        throw std::out_of_range("VariableLengthArray::at");
#else
        std::abort();
#endif
    }
    return container[pos];
}

@pavel-kirienko

Copy link
Copy Markdown
Member

I normally use https://github.com/marketplace/actions/clang-format-lint together with a format build target that invokes clang-format when necessary, also there are IDE integrations for on-the-fly reformatting. If you want to keep the formatting check strictly a part of CMake project, then perhaps invoking clang-format with clang-format --dry-run --Werror <paths...> is what you need?

@pavel-kirienko

Copy link
Copy Markdown
Member

Sorry, but I'm not going to put a production abort() into the code. You can implement an external helper:

How about the old at_or_null? For the bool specialization it would become:

std::optional<reference> at_or_null(const size_type);
std::optional<const_reference> at_or_null(const size_type) const;

Comment thread cetlvast/suites/docs/footer.html Outdated
@thirtytwobits

Copy link
Copy Markdown
Member Author

Sorry, but I'm not going to put a production abort() into the code. You can implement an external helper:

How about the old at_or_null? For the bool specialization it would become:

std::optional<reference> at_or_null(const size_type);
std::optional<const_reference> at_or_null(const size_type) const;

Yes. I was planning on doing that when I added optional to CETL.

Comment thread cetlvast/cmake/modules/Findgoogletest.cmake
Comment thread include/cetl/variable_length_array.hpp
Comment thread include/cetl/variable_length_array.hpp
Comment thread include/cetl/variable_length_array.hpp
Fixing CI to properly report test coverage and to actually execute the compile tests.
@thirtytwobits

Copy link
Copy Markdown
Member Author

Sorry for the rebase but I wanted squash all the crap I've been picking away at so I can add the few substantive changes still pending on top of a single "fix CI" change.

IMG_4333

@thirtytwobits
thirtytwobits requested review from pavel-kirienko and skeetsaz and removed request for lydia-at-amazon June 6, 2023 16:26
@thirtytwobits

Copy link
Copy Markdown
Member Author

@pavel-kirienko ? Can you approve?

@sonarqubecloud

sonarqubecloud Bot commented Jun 7, 2023

Copy link
Copy Markdown

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

91.4% 91.4% Coverage
0.6% 0.6% Duplication

@thirtytwobits
thirtytwobits merged commit 71e0ccd into main Jun 7, 2023
@thirtytwobits
thirtytwobits deleted the develop/vla branch June 7, 2023 15:01
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