Skip to content

Extend NonRelocatable vector API - #599

Merged
4og merged 2 commits into
eclipse-score:mainfrom
qorix-group:pawelrutkaq_non_relocatable_vector_api
Sep 21, 2026
Merged

Extend NonRelocatable vector API#599
4og merged 2 commits into
eclipse-score:mainfrom
qorix-group:pawelrutkaq_non_relocatable_vector_api

Conversation

@pawelrutkaq

@pawelrutkaq pawelrutkaq commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Added API:

  • pop_back
  • clear
  • empty

@github-project-automation github-project-automation Bot moved this to In Progress in BAS - Baselibs FT Sep 18, 2026
@github-actions github-actions Bot added comp-cpp-containers Related to score/containers c++ C++ code bazel Bazel and Starlark build files labels Sep 18, 2026
@pawelrutkaq
pawelrutkaq force-pushed the pawelrutkaq_non_relocatable_vector_api branch from 6eb46d6 to 74bd619 Compare September 18, 2026 10:00
arkjedrz
arkjedrz previously approved these changes Sep 18, 2026
View reviewed changes
@github-project-automation github-project-automation Bot moved this from In Progress to On Hold in BAS - Baselibs FT Sep 18, 2026
@github-actions

github-actions Bot commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Documentation preview for this pull request is available at:
pr-599: https://eclipse-score.github.io/baselibs/pr-599/

@4og

4og commented Sep 18, 2026

Copy link
Copy Markdown
Member

@fbaeuerle, @mihajlo-k, @Nikhil2206 can some of you review this PR?

mihajlo-k
mihajlo-k approved these changes Sep 18, 2026
View reviewed changes

@mihajlo-k mihajlo-k left a comment

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.

LGTM.

btw, out of the scope here, but looking at these changes I think we could add [[nodiscard]] on some other methods as well. Also zero the size and capacity on move assignment.

@4og 4og mentioned this pull request Sep 18, 2026
Open
@4og

4og commented Sep 18, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

LGTM.

btw, out of the scope here, but looking at these changes I think we could add [[nodiscard]] on some other methods as well. Also zero the size and capacity on move assignment.

I added an issue: #601

@4og
4og added this pull request to the merge queue Sep 18, 2026
@4og
4og removed this pull request from the merge queue due to a manual request Sep 18, 2026
@4og
4og requested a balanced review from Copilot September 18, 2026 15:40
Copilot AI reviewed Sep 18, 2026
View reviewed changes

Copilot AI left a comment

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.

🟡 Changes recommended

Required test traceability metadata and repository naming, documentation, and copyright conventions must be addressed.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Extends NonRelocatableVector with standard capacity-management operations and corresponding tests.

Changes:

  • Adds pop_back(), clear(), and empty().
  • Resets moved-from state and centralizes element destruction.
  • Adds comprehensive API tests and a Bazel test target.
File summaries
File Description
score/containers/non_relocatable_vector.h Implements the new APIs and destruction helper.
score/containers/non_relocatable_vector_special_member_funcs_test.cpp Tests moved-from size and capacity.
score/containers/non_relocatable_vector_capacity_test.cpp Tests empty, pop-back, and clear behavior.
score/containers/BUILD Registers the new test target.
Review details

Suppressed comments (4)

score/containers/non_relocatable_vector.h:148

  • The new Doxygen tags use backslash syntax, while the C++ style guide requires @-prefixed tags (docs/cpp-style-guide.md:54-56).
    /// \brief Destroys every element, reducing size() to zero.
    /// \note capacity() is unchanged and no memory is deallocated.

score/containers/non_relocatable_vector.h:149

  • clear is a newly introduced method, but repository C++ methods must use PascalCase (docs/cpp-style-guide.md:27-28). Rename it to Clear and update its definition and callers.
    void clear() noexcept;

score/containers/non_relocatable_vector.h:164

  • The new Doxygen tag uses backslash syntax, while the C++ style guide requires the @brief form (docs/cpp-style-guide.md:54-56).
    /// \brief Checks whether the vector has no elements.

score/containers/non_relocatable_vector.h:197

  • The new Doxygen tag uses backslash syntax, while the C++ style guide requires the @brief form (docs/cpp-style-guide.md:54-56).
    /// \brief Destroys every constructed element, leaving size_ and the underlying storage untouched.
  • Files reviewed: 4/4 changed files
  • Comments generated: 6
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread score/containers/non_relocatable_vector.h
Comment thread score/containers/non_relocatable_vector.h
Comment thread score/containers/non_relocatable_vector.h
Comment thread score/containers/non_relocatable_vector_capacity_test.cpp
Comment thread score/containers/non_relocatable_vector_capacity_test.cpp
Comment thread score/containers/non_relocatable_vector_special_member_funcs_test.cpp
}
}

TYPED_TEST(NonRelocatableVectorTrivialFixture, MoveConstructingLeavesSourceWithZeroSizeAndCapacity)

@4og 4og Sep 18, 2026

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.

Agree that this needs to be done. There also a PR already cleaning up the containers lib. #568

@pawelrutkaq pawelrutkaq Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

@pawelrutkaq

pawelrutkaq commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Also zero the size and capacity on move assignment.

I will not do the move assignment in this PR since it has a bigger scope, impacting Dynamic Array contracts

@pawelrutkaq
pawelrutkaq force-pushed the pawelrutkaq_non_relocatable_vector_api branch from cd837b4 to 2af4ae7 Compare September 21, 2026 06:13
@pawelrutkaq
pawelrutkaq requested a review from 4og September 21, 2026 06:13
@4og
4og added this pull request to the merge queue Sep 21, 2026
Merged via the queue into eclipse-score:main with commit 896d501 Sep 21, 2026
27 checks passed
@github-project-automation github-project-automation Bot moved this from On Hold to Done in BAS - Baselibs FT Sep 21, 2026

This branch was previously deployed

1 inactive deployment
workflow-approval — 2af4ae7f Deployed Sep 21, 2026 by pawelrutkaq via Build and Test bl-aarch64-qnx / approval #2004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@4og 4og 4og approved these changes
@arkjedrz arkjedrz arkjedrz left review comments
@antonkri antonkri Awaiting requested review from antonkri antonkri is a code owner
@fbaeuerle fbaeuerle Awaiting requested review from fbaeuerle fbaeuerle is a code owner
+1 more reviewer
@mihajlo-k mihajlo-k mihajlo-k approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bazel Bazel and Starlark build files c++ C++ code comp-cpp-containers Related to score/containers

Projects

BAS - Baselibs FT
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@pawelrutkaq @4og @arkjedrz @mihajlo-k