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

Gcc12 related fixes #1138

Merged
merged 4 commits into from
Jun 9, 2023
Merged

Conversation

u-foka
Copy link
Contributor

@u-foka u-foka commented Jun 9, 2023

Make the code compile with gcc12

  • Compile with -std=c++20 when available
  • Missing includes
  • Little fixes
  • Suppress volatile warnings

Switch to c++20 was necessary due to the usage of std::string.c_str() inside constexpr code. This only became standard in c++20 and the new gcc complains about it when trying to compile with -std=c++17 :(

To preserve the ability of compiling with older gcc I used check_cxx_compiler_flag and only enable c++20 if the compiler can handle it. Tested and works with gcc9.2 and gcc12.2, would be great if you could test it with in-between versions!

u-foka added 4 commits June 9, 2023 18:22
On gcc12 we need to use -std=c++20 since constexpr .c_str() on std::string is only officially available since c++20 and the new gcc wouldnt let us use it with older standard
Copy link
Contributor

@Brumi-2021 Brumi-2021 left a comment

Choose a reason for hiding this comment

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

I could also compile correctly with gcc-arm-...version 10.3 .
So , no compile errors , and those changes for next gcc v 12.2 are also compatible with mine gcc v 10.3.
All seems working perfectly .
But looking some code changes on TPMS App, I do not know , maybe it would be better if someone , could also confirm that this App has not any side effect.

So to me , it is approved , but better if someone can also in parallel validate correct TPMS App functionality of that binary ,

Thanks for that future preparation to an smooth migration to next gcc-arm -... v 12.2

Note : Prior to that gcc version 12.2 related fixes ,
There were issued another PR #1135 , the first gcc-arm-none-eabi common gcc 10.3 and 12.2 related fixes without that PR , persistent memory UI was not working from 10.3 onwards, now solved )

Copy link
Contributor

@jLynx jLynx left a comment

Choose a reason for hiding this comment

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

Does the docker containers work, or do you need to update those?

@u-foka
Copy link
Contributor Author

u-foka commented Jun 9, 2023

So to me , it is approved , but better if someone can also in parallel validate correct TPMS App functionality of that binary ,

Hy, all I've done is move the assignment operator to the header, inside the class. Somehow the compiler didnt use it otherwise. This solved it. The class already had members defined there, so I'm not too afraid of runtime sideeffects, if it compiles it should work ;) Especially with the old compiler.

What potentially can have sideeffects is using a newer compiler.. We've caught the obvious one with pmem, but there are potentially more sneaky ones. So this PR is to enable easier testing and development using newer compiler to gain more experience with it, test bins with new compilers more :)

Next step could be having nightlies with both, test them extensively, then later make it default for nightlies test, test, test and then eventually make a first release with it :)

But before that happens we probably need to look at the binary size issue too..

@u-foka
Copy link
Contributor Author

u-foka commented Jun 9, 2023

Does the docker containers work, or do you need to update those?

This still fully compatible with gcc9.2 so no need to immediately upgrade the official toolchain, I would not rush that.

Copy link
Contributor

@jLynx jLynx left a comment

Choose a reason for hiding this comment

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

Ah gotcha. This is just getting it ready for 12

@jLynx jLynx merged commit a2e5e03 into portapack-mayhem:next Jun 9, 2023
@u-foka u-foka deleted the gcc12-related-fixes branch June 9, 2023 20:36
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