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

Reapply CPP20 compatibility and enable check in CI #28

Merged
merged 4 commits into from
Jan 7, 2025

Conversation

ThoFrank
Copy link
Contributor

Changes from previous PR #8 were overwritten. This reapplies them and enables CI builds for cpp 14,17 and 20.

@ThoFrank
Copy link
Contributor Author

@mthiede-acn2 could you please take a look? The changes from #8 are all done in one commit, but I had to patch one more file.

@@ -8,6 +8,7 @@ jobs:
strategy:
matrix:
platform: [posix, s32k148]
cpp-standard: [14, 17, 20]
Copy link
Contributor

Choose a reason for hiding this comment

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

this will of course extend our build times. Would probably be enough to just run the compiler front end without generating code. We can use the compiler’s syntax-checking mode or run only the front-end to parse by using the -fsyntax-only flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think -fsyntax-only is supported by CMake. It would probably require a bigger hack. I tried it naively by adding -DCMAKE_C_FLAGS which resulted in CMake complaining about a broken compiler which doesn't produce binaries.

We could add ccache in the future. This could help bring down compilation times. Somebody already created a ccache-action which combines ccache with github's cache.

@marcmo
Copy link
Contributor

marcmo commented Dec 12, 2024

Regarding the C++ 20 compatibility changes, some of them seem to be highly controversial.
why is removing volatile valid in all those places? we cannot just remove it!
getting rid of volatile AND the compound assignment for sure gets rid of the warnings, but it changes the code.

and..does it really make sense to get rid of compound assignments on volatile anyway?
I'd rather NOT add C++ 20 support as long as we don't have the need...and then only do it after careful consideration. @matthiaskessler ?

Copy link
Contributor

@marcmo marcmo left a comment

Choose a reason for hiding this comment

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

do not just remove volatile

@ThoFrank
Copy link
Contributor Author

I just reapplied the changes from #8. They were already approved and merged before the migration to GitHub.
So I didn't think it would be controversial. I combined all the changes from #8 into one commit. My changes are done in other commits

@marcmo marcmo merged commit 63f00d2 into eclipse-openbsw:main Jan 7, 2025
10 checks passed
@ThoFrank ThoFrank deleted the ci_cpp20 branch January 7, 2025 11:41
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.

2 participants