Fix pgm_read_ptr in AVR pgmspace.h

[tttapa]

Originally, pgm_read_ptr used to cast its argument to const void *, i.e. a pointer to read-only data.
This is incorrect, because the argument is a pointer to a pointer in "flash memory". You cannot cast it to
const void * and then dereference it, because void is not an object type.
Also, the pointer itself is read-only, but the data could in theory be mutable.
The correct type should be void *const *, i.e. a pointer to a read-only pointer to any data.

[bblanchon]

I confirm that this change is necessary to fix the following error:

arduino/api/deprecated-avr-comp/avr/pgmspace.h:107:49: error: 'const void*' is not a pointer-to-object type
 #define pgm_read_ptr(addr) (*(const void *)(addr))
                                                 ^

Please merge this pull request.

[matthijskooijman]

Also looks good to me.

One nitpick: I would have a small preference to use const void ** instead of void *const *, which is a bit easier to read but AFAIU equivalent. This is also what is already used in practice in e.g. the SAMD core: https://github.com/adafruit/ArduinoCore-samd/blob/6a59e8347d01a9a7a51cd71f7981d154d851ba35/cores/arduino/avr/pgmspace.h#L106

[bblanchon]

The core for ESP32 use void * const *.
https://github.com/espressif/arduino-esp32/blob/37a7fb3d6ada8a9230c96e2df999c95ef10fd4d3/cores/esp32/pgmspace.h#L53

[bblanchon]

The core for ESP8266 uses const void* const*
https://github.com/esp8266/Arduino/blob/40eb5747e4b7d3458e9406c3c446948bbffa767b/tools/sdk/libc/xtensa-lx106-elf/include/sys/pgmspace.h#L115

[bblanchon]

stm32duino uses void * const *
https://github.com/stm32duino/Arduino_Core_STM32/blob/70a6928ef65d7a8881b05e12f866a214898ac48e/cores/arduino/avr/pgmspace.h#L115

[bblanchon]

"Arduino STM32" uses const void **
https://github.com/rogerclarkmelbourne/Arduino_STM32/blob/fedf4d4c3bfdc2377b20dc0d8e5339bb6c505fbe/STM32F1/cores/maple/avr/pgmspace.h#L35

[bblanchon]

As you see, there are many variants out there.
However, if we go back to the original definition in avr-libc, we see that the macro casts the result to void*.
Therefore, to emulate the original macro, we should use void *const *, as @tttapa wrote.

[matthijskooijman]

But I'm saying that, AFAIU, const void** is exactly the same type as void *const *, it's just written in an IMHO more readable/common way. Or are you arguing that these types are different?

[bblanchon]

It's not a matter of taste; they are different types:

• *(const void**) is const void*
• *(void *const *) is void*

[matthijskooijman]

Hm, seems I was wrong indeed, thanks for insisting.

So with that out of the way, the PR as it is now is indeed correct, so I'm for merging it.

Note that this also means that the SAMD and Arduino_STM32 versions are indeed also subtly broken (returning const void* instead of void*), though not as broken as the version in the ArduinoCore-API repo right now (which does not even dereference successfully).

[aentinger]

Thank you to both @bblanchon and @matthijskooijman 🚀 I'll merge now.

[bblanchon]

Thanks !!!