Skip to content
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.

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

Fix issues: allocator passing, verification test C++ flavors, union code-gen #323

Merged
merged 5 commits into from
Nov 28, 2023

Conversation

skeetsaz
Copy link
Contributor

  • Changed the verification tests CMakeLists.txt so it uses a matrix of test files to language flavors. This allows turning language flavors on or off for each test.
  • Updated the docker image tag used by the verification tests.
  • Fixed the c++17-pmr flavor to use "memory_resource" as the allocator include since that's where std::pmr::polymorphic_allocator is.
  • Exclude unions from all code-gen using allocator since there is not a way to pass it through to the elements of std::variant in a way that makes sense.

…ode-gen

- Changed the verification tests CMakeLists.txt so it uses a matrix of test
  files to language flavors.  This allows turning language flavors on or off
  for each test.
- Updated the docker image tag used by the verification tests.
- Fixed the c++17-pmr flavor to use "memory_resource" as the allocator include
  since that's where std::pmr::polymorphic_allocator is.
- Exclude unions from all code-gen using allocator since there is not a way
  to pass it through to the elements of std::variant in a way that makes sense.
@pavel-kirienko
Copy link
Member

Exclude unions from all code-gen using allocator since there is not a way to pass it through to the elements of std::variant in a way that makes sense.

Could you please expand a bit on the implications of this for user code? Will one still be able to use unions with pmr?

@skeetsaz
Copy link
Contributor Author

I looked into using unions with PMR and it looks like it is indeed do-able. I worked out what the code-gen should look like here. There are a few subtleties but I think it's a good way to go.
https://godbolt.org/z/qvWTnhEd8

If you don't mind, I'd like to move forward with what I have here and do the union work in a separate PR. The current code checked into github is broken (not sure why the github automated tests aren't running everything but if you pull it down locally and run all the verification tests they are broken.) This will get them all compiling/passing again.

Thanks for bringing this up. I was not too familiar with std::variant so it forced me to read up on it and understand it better.

@pavel-kirienko
Copy link
Member

Okay. I suggest setting the version tag to 2.3.2.dev0 here (with the dev suffix) to avoid side effects on the user side until the follow-up fix is ready.

Copy link

sonarcloud bot commented Nov 22, 2023

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

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@skeetsaz
Copy link
Contributor Author

All the tests pass except for readthedocs which seems to be broken due to some infrastructure issue.

@thirtytwobits thirtytwobits merged commit 256fb06 into OpenCyphal:main Nov 28, 2023
182 of 183 checks passed
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