ArduinoIoTCloud icon indicating copy to clipboard operation
ArduinoIoTCloud copied to clipboard

Avoid creating constants and macros in the global namespace

Open alranel opened this issue 2 years ago • 6 comments

This library defines some constants in the global namespace, such as the following ones:

https://github.com/arduino-libraries/ArduinoIoTCloud/blob/451d57d53da33dfad490c6c0cd9f521780066e7c/src/ArduinoIoTCloud.h#L51-L56

This generates errors whenever they are also defined by other libraries, preventing their coexistence. For instance, ~the ArduinoBLE library defines both READ and WRITE~ (not anymore, constants were renamed in ArduinoBLE) and in general it is very likely to find constants named like this.

It would be nice to avoid polluting the global namespace, obviously in a retrocompatible way...

alranel avatar Apr 24 '22 21:04 alranel

Another example of this practice resulting in name collisions: https://github.com/arduino-libraries/ArduinoIoTCloud/issues/280

per1234 avatar Jul 01 '22 02:07 per1234

We also have this problem with macros such as the ones defined here:

https://github.com/arduino-libraries/ArduinoIoTCloud/blob/925922bc73929eadf14fd79199817c188676e606/src/property/Property.h#L50-L52

setAttributes() is a very common function name, and in fact this prevents using the very popular TFT_eSPI library with IoT Cloud as compilation fails as follows:

In file included from /tmp/936788285/TTGO_oct06a/TTGO_oct06a.ino:19: /home/builder/opt/libraries/latest/tft_espi_2_4_72/TFT_eSPI.h:728:54: error: macro "setAttribute" passed 2 arguments, but takes just 1 void setAttribute(uint8_t id = 0, uint8_t a = 0); // Set attribute value ^ Error during build: exit status 1 In file included from /tmp/936788285/TTGO_oct06a/TTGO_oct06a.ino:19: /home/builder/opt/libraries/latest/tft_espi_2_4_72/TFT_eSPI.h:728:54: error: macro "setAttribute" passed 2 arguments, but takes just 1 void setAttribute(uint8_t id = 0, uint8_t a = 0); // Set attribute value ^ Error during build: exit status 1

The workaround is to undef the symbol in the user sketch before including other libraries:

#undef setAttribute(x)
#include <TFT_eSPI.h>

However this library should be fixed to maximize its compatibility and avoid any overlap with other symbols in the global namespace.

alranel avatar Oct 06 '22 17:10 alranel

Since the macro is only being used internally inside the various Cloud* types, I suggest to undef at the end of all .h using it (https://github.com/arduino-libraries/ArduinoIoTCloud/search?q=setAttribute) @pennam can you take a look?

facchinm avatar Oct 07 '22 09:10 facchinm

@facchinm With that approach wouldn't you still override any existing macros created by other libraries imported before this one? Wouldn't it be better to switch to namespaced symbols such as proper functions, constexprs or even just prefixing the macros with AIOTC_?

alranel avatar Oct 12 '22 11:10 alranel

These symbols cannot be turned into proper functions due to stringification, but since they are not intended to be used by the end user we can safely prefix with AIOTC_ or similar

facchinm avatar Oct 12 '22 12:10 facchinm

@alranel regarding READ, WRITE and READWRITE there is already an alternative in the library:

https://github.com/arduino-libraries/ArduinoIoTCloud/blob/9c6d5d741ea936009f0e538deb33bfc655ca02e3/src/property/Property.h#L130-L132

The first step to remove READ, WRITE and READWRITE is to update the template used to generate the sketches in the cloud editor, but unfortunately even if we do so, removing them from the library will be a breaking change becase all "non-cloud" sketches will throw a compilation error.

/cc @eclipse1985

pennam avatar Nov 10 '22 16:11 pennam