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 build warnings/errors in C++20 (#2883) #1

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

berhoel
Copy link
Owner

@berhoel berhoel commented Sep 23, 2024

FlashString static initialisation

The Object copy and move constructors are deleted as a precaution as these objects should not be copied. C++20 doesn't like this so they're only compiled for C++17 and earlier.

std:is_pod deprecated

Use std::is_standard_layout instead (only one instance to correct).

Volatile

C++20 deprecates many uses of volatile which may be implemented in an ambiguous (non-deterministic) way. Largely these uses are concerned with threaded applications. A general web search turns up plenty of articles on this.

One argument is related to code like this:

volatile int num;
++num; // Is this atomic? Or a read, followed by load?

So we should identify all such cases and code them more explicitly.

The FIFO/FILO classes define numberOfElements as volatile but I can see no reason why this needs to be the case.

Volatile bitwise operations

C++23 will, it seems, de-deprecate use of volatile for bitwise operations. This is discussed here https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2327r1.pdf. See also earlephilhower/arduino-pico#1379

These are genuine uses of volatile as values are used both in interrupt and task context. Some of these actually need to be atomic, and the assumption is probably that x |= n does that. Hence the ambiguity. These are the core Sming files affected:

  • Sming/Arch/Esp8266/Components/driver/i2s.cpp
  • Sming/Arch/Esp8266/Components/spi_flash/iram_precache.cpp
  • Sming/Arch/Esp8266/Components/gdbstub/appcode/gdb_hooks.cpp
  • Sming/Arch/Esp8266/Components/gdbstub/gdbstub.cpp
  • Sming/Arch/Esp8266/Components/gdbstub/gdbuart.cpp
  • Sming/Arch/Esp8266/Core/Digital.cpp

There will be many others in libraries.

Solution: Rather than attempting to 'correct' the code and introduce bugs - risks identified in the above paper - it is safer to just silence this particular warning. This has been done in the main Sming build.mk file so it's easy to revert if required. This is the volatile warning which covers other uses as well - future GCC revisions may add some granularity to this.

FIFO / FILO uses int instead of unsigned

No warnings but using int for the count makes no sense. Not related to C++20 but since we're modifying the classes anyway may as well do it here.

**FlashString static initialisation**

The `Object` copy and move constructors are deleted as a precaution as these objects should not be copied.
C++20 doesn't like this so they're only compiled for C++17 and earlier.

**std:is_pod deprecated**

Use std::is_standard_layout instead (only one instance to correct).

**Volatile**

C++20 deprecates many uses of `volatile` which may be implemented in an ambiguous (non-deterministic) way.
Largely these uses are concerned with threaded applications. A general web search turns up plenty of articles on this.

One argument is related to code like this:

```c++
volatile int num;
++num; // Is this atomic? Or a read, followed by load?
```

So we should identify all such cases and code them more explicitly.

The FIFO/FILO classes define `numberOfElements` as volatile but I can see no reason why this needs to be the case.

**Volatile bitwise operations**

C++23 will, it seems, de-deprecate use of volatile for bitwise operations.
This is discussed here https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/p2327r1.pdf.
See also earlephilhower/arduino-pico#1379

These are genuine uses of volatile as values are used both in interrupt and task context. Some of these actually need to be atomic, and the assumption is probably that `x |= n` does that. Hence the ambiguity. These are the core Sming files affected:

* Sming/Arch/Esp8266/Components/driver/i2s.cpp
* Sming/Arch/Esp8266/Components/spi_flash/iram_precache.cpp
* Sming/Arch/Esp8266/Components/gdbstub/appcode/gdb_hooks.cpp
* Sming/Arch/Esp8266/Components/gdbstub/gdbstub.cpp
* Sming/Arch/Esp8266/Components/gdbstub/gdbuart.cpp
* Sming/Arch/Esp8266/Core/Digital.cpp

There will be many others in libraries.

Solution: Rather than attempting to 'correct' the code and introduce bugs - risks identified in the above paper - it is safer to just silence this particular warning. This has been done in the main Sming `build.mk` file so it's easy to revert if required. This is the `volatile` warning which covers other uses as well - future GCC revisions may add some granularity to this.


**FIFO / FILO uses int instead of unsigned**

No warnings but using `int` for the count makes no sense.
Not related to C++20 but since we're modifying the classes anyway may as well do it here.
@berhoel berhoel merged commit 7aae7e1 into berhoel:feature/allow-user-install Sep 23, 2024
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