Skip to content

C++: Updated constructor generation and allocator handling for unions #383

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
thirtytwobits merged 2 commits into from
Aug 4, 2025
Merged

C++: Updated constructor generation and allocator handling for unions #383

thirtytwobits merged 2 commits into from
Aug 4, 2025

Conversation

@JLReitz
Copy link

@JLReitz JLReitz commented Jul 10, 2025

  • Allocators are stored and propagated to union composite type alternative values during construction and emplacement
  • in_place_index constructor is generated for union composite types
  • DependencyBuilder sets uses_union flag appropriately

@JLReitz JLReitz force-pushed the unionant-allocator-propogation branch 2 times, most recently from 448aaf9 to aa2849f Compare July 11, 2025 19:42
@JLReitz JLReitz marked this pull request as ready for review July 11, 2025 19:56
@JLReitz
Copy link
Author

JLReitz commented Jul 11, 2025

How can we enable pmr-flavored language checks for this PR?

@JLReitz JLReitz mentioned this pull request Jul 11, 2025
Open
@thirtytwobits
Copy link
Member

thirtytwobits commented Jul 14, 2025

How can we enable pmr-flavored language checks for this PR?

In .github/workflows see the test.yml. Look for the language-verification-c-cpp: section and then strategy/matrix/language under that section. Add the appropriate PMR overrides to this array and the CI will run.

JLReitz
JLReitz commented Jul 14, 2025
View reviewed changes
JLReitz
JLReitz commented Jul 14, 2025
View reviewed changes
src/nunavut/lang/cpp/templates/_composite_type.j2

{%- if composite_type.inner_type is not UnionType %}
{% if composite_type.fields_except_padding %}
// Initializing constructor
Copy link
Author

@JLReitz JLReitz Jul 14, 2025

Choose a reason for hiding this comment

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

At the moment tagged in_place_index constructors are only defined when the language options have constructor_convention != default, FYI

@JLReitz JLReitz force-pushed the unionant-allocator-propogation branch from aa2849f to 318a314 Compare July 15, 2025 02:46
@JLReitz
Copy link
Author

JLReitz commented Jul 15, 2025 •
edited
Loading

Interesting failure that popped up all of a sudden during the lint test.

lint: commands[3]> mypy -p nunavut --cache-dir /__w/nunavut/nunavut/.tox/lint/tmp --txt-report /__w/nunavut/nunavut/.tox/lint/tmp/mypy-report-lib --config-file /__w/nunavut/nunavut/tox.ini
src/nunavut/cli/__init__.py: note: In function "_make_parser":
src/nunavut/cli/__init__.py:916: error: Redundant cast to "ParserT"  [redundant-cast]

Could it be from me updating .github/workflows/test.py? Looks like I'm not out of sync with the 3.0.preview branch. I ran the full tox suite within an up to date python development container and could not reproduce.

@thirtytwobits
Copy link
Member

thirtytwobits commented Jul 15, 2025

Interesting failure that popped up all of a sudden during the lint test.

lint: commands[3]> mypy -p nunavut --cache-dir /__w/nunavut/nunavut/.tox/lint/tmp --txt-report /__w/nunavut/nunavut/.tox/lint/tmp/mypy-report-lib --config-file /__w/nunavut/nunavut/tox.ini
src/nunavut/cli/__init__.py: note: In function "_make_parser":
src/nunavut/cli/__init__.py:916: error: Redundant cast to "ParserT"  [redundant-cast]

Could it be from me updating .github/workflows/test.py? Looks like I'm not out of sync with the 3.0.preview branch. I ran the full tox suite within an up to date python development container and could not reproduce.

.github/workflows/test.py doesn't exist anymore. Looks like this is something from a change upstream. Try changing line 916 of src/nunavut/cli/init.py to be just return parser

@JLReitz
Copy link
Author

JLReitz commented Jul 16, 2025 •
edited
Loading

.github/workflows/test.py doesn't exist anymore.

My mistake, I had meant .github/workflows/test.yml

@JLReitz JLReitz force-pushed the unionant-allocator-propogation branch from 318a314 to 4428bd5 Compare July 16, 2025 20:29
@JLReitz
Copy link
Author

JLReitz commented Jul 30, 2025 •
edited
Loading

Bugfix for CETL VariableLengthArray required to resolve failing checks: OpenCyphal/CETL#188

@emrainey emrainey requested a review from thirtytwobits July 31, 2025 17:06
@JLReitz
Copy link
Author

JLReitz commented Jul 31, 2025

Waiting for the release of CETL v.1.4.2 to push submodule update

@thirtytwobits
Copy link
Member

thirtytwobits commented Aug 1, 2025

Waiting for the release of CETL v.1.4.2 to push submodule update

https://github.com/OpenCyphal/CETL/releases/tag/v1.4.2

JLReitz added 2 commits August 1, 2025 17:28
@JLReitz
C++: Updated constructor generation and allocator handling for unions
442f457 
 - Allocators are stored and propagated to union composite type alternative values during construction and emplacement
 - `in_place_index` constructor is generated for union composite types
 - `DependencyBuilder` sets `uses_union` flag appropriately
@JLReitz
Checking out CETL v1.4.2
2f193f3
@JLReitz JLReitz force-pushed the unionant-allocator-propogation branch from 4428bd5 to 2f193f3 Compare August 1, 2025 17:29
@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 1, 2025

Quality Gate Passed Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@thirtytwobits
Copy link
Member

thirtytwobits commented Aug 4, 2025

Strong work. Thank you for your contribution citizen. 🇺🇸🏈🥧

@thirtytwobits thirtytwobits merged commit 426fe05 into OpenCyphal:3.0.preview Aug 4, 2025
60 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thirtytwobits thirtytwobits thirtytwobits approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@JLReitz @thirtytwobits