Skip to content

Commit

Permalink
Fix build warnings/errors in C++20 (#2883)
Browse files Browse the repository at this point in the history
**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.
  • Loading branch information
mikee47 authored Sep 23, 2024
1 parent 3cb6f28 commit e6ed3d9
Show file tree
Hide file tree
Showing 5 changed files with 27 additions and 22 deletions.
2 changes: 1 addition & 1 deletion Sming/Components/FlashString
2 changes: 1 addition & 1 deletion Sming/Components/Storage/src/include/Storage/Partition.h
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ class Partition
bool read(storage_size_t offset, void* dst, size_t size);

template <typename T>
typename std::enable_if<std::is_pod<T>::value, bool>::type read(storage_size_t offset, T& value)
typename std::enable_if<std::is_standard_layout<T>::value, bool>::type read(storage_size_t offset, T& value)
{
return read(offset, &value, sizeof(value));
}
Expand Down
20 changes: 10 additions & 10 deletions Sming/Wiring/FIFO.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,10 @@

#include "Countable.h"

template <typename T, int rawSize> class FIFO : public Countable<T>
template <typename T, unsigned rawSize> class FIFO : public Countable<T>
{
public:
const int size; // speculative feature, in case it's needed
const unsigned size; // speculative feature, in case it's needed

FIFO();

Expand Down Expand Up @@ -60,18 +60,18 @@ template <typename T, int rawSize> class FIFO : public Countable<T>
}

protected:
volatile int numberOfElements;
int nextIn;
int nextOut;
unsigned numberOfElements;
unsigned nextIn;
unsigned nextOut;
T raw[rawSize];
};

template <typename T, int rawSize> FIFO<T, rawSize>::FIFO() : size(rawSize)
template <typename T, unsigned rawSize> FIFO<T, rawSize>::FIFO() : size(rawSize)
{
flush();
}

template <typename T, int rawSize> bool FIFO<T, rawSize>::enqueue(T element)
template <typename T, unsigned rawSize> bool FIFO<T, rawSize>::enqueue(T element)
{
if(full()) {
return false;
Expand All @@ -85,7 +85,7 @@ template <typename T, int rawSize> bool FIFO<T, rawSize>::enqueue(T element)
return true;
}

template <typename T, int rawSize> T FIFO<T, rawSize>::dequeue()
template <typename T, unsigned rawSize> T FIFO<T, rawSize>::dequeue()
{
T item;
numberOfElements--;
Expand All @@ -95,12 +95,12 @@ template <typename T, int rawSize> T FIFO<T, rawSize>::dequeue()
return item;
}

template <typename T, int rawSize> T FIFO<T, rawSize>::peek() const
template <typename T, unsigned rawSize> T FIFO<T, rawSize>::peek() const
{
return raw[nextOut];
}

template <typename T, int rawSize> void FIFO<T, rawSize>::flush()
template <typename T, unsigned rawSize> void FIFO<T, rawSize>::flush()
{
nextIn = nextOut = numberOfElements = 0;
}
Expand Down
20 changes: 10 additions & 10 deletions Sming/Wiring/FILO.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@

#include "Countable.h"

template <typename T, int rawSize> class FILO : public Countable<T>
template <typename T, unsigned rawSize> class FILO : public Countable<T>
{
public:
const int size; // speculative feature, in case it's needed
const unsigned size; // speculative feature, in case it's needed

FILO();

Expand Down Expand Up @@ -59,18 +59,18 @@ template <typename T, int rawSize> class FILO : public Countable<T>
}

private:
volatile int numberOfElements;
int nextIn;
int nextOut;
unsigned numberOfElements;
unsigned nextIn;
unsigned nextOut;
T raw[rawSize];
};

template <typename T, int rawSize> FILO<T, rawSize>::FILO() : size(rawSize)
template <typename T, unsigned rawSize> FILO<T, rawSize>::FILO() : size(rawSize)
{
flush();
}

template <typename T, int rawSize> bool FILO<T, rawSize>::push(T element)
template <typename T, unsigned rawSize> bool FILO<T, rawSize>::push(T element)
{
if(count() >= rawSize) {
return false;
Expand All @@ -79,23 +79,23 @@ template <typename T, int rawSize> bool FILO<T, rawSize>::push(T element)
return true;
}

template <typename T, int rawSize> T FILO<T, rawSize>::pop()
template <typename T, unsigned rawSize> T FILO<T, rawSize>::pop()
{
if(numberOfElements > 0) {
return raw[--numberOfElements];
}
return raw[0];
}

template <typename T, int rawSize> T FILO<T, rawSize>::peek() const
template <typename T, unsigned rawSize> T FILO<T, rawSize>::peek() const
{
if(numberOfElements > 0) {
return raw[numberOfElements - 1];
}
return raw[0];
}

template <typename T, int rawSize> void FILO<T, rawSize>::flush()
template <typename T, unsigned rawSize> void FILO<T, rawSize>::flush()
{
nextIn = nextOut = numberOfElements = 0;
}
5 changes: 5 additions & 0 deletions Sming/build.mk
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,11 @@ COMPILER_VERSION_FULL := $(shell LANG=C $(CC) -v 2>&1 | $(AWK) -F " version " '/
COMPILER_NAME := $(word 1,$(COMPILER_VERSION_FULL))
COMPILER_VERSION := $(word 2,$(COMPILER_VERSION_FULL))

# Use of bitwise assignment for volatile registers is deprecated in C++20 but de-deprecated in C++23
ifeq ($(COMPILER_NAME)-$(SMING_CXX_STD),gcc-c++20)
CXXFLAGS += -Wno-volatile
endif

ifndef USE_CLANG
# Required to access peripheral registers using structs
# e.g. `uint32_t value: 8` sitting at a byte or word boundary will be 'optimised' to
Expand Down

0 comments on commit e6ed3d9

Please sign in to comment.