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

Avoid creating constants and macros in the global namespace #315

Open
alranel opened this issue Apr 24, 2022 · 6 comments · Fixed by #339
Open

Avoid creating constants and macros in the global namespace #315

alranel opened this issue Apr 24, 2022 · 6 comments · Fixed by #339
Labels
topic: code Related to content of the project itself

Comments

@alranel
Copy link
Member

alranel commented Apr 24, 2022

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

typedef enum
{
READ = 0x01,
WRITE = 0x02,
READWRITE = READ | WRITE
} permissionType;

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...

@per1234 per1234 added the topic: code Related to content of the project itself label Apr 24, 2022
@per1234
Copy link
Contributor

per1234 commented Jul 1, 2022

Another example of this practice resulting in name collisions: #280

@alranel alranel changed the title Avoid creating constants in the global namespace Avoid creating constants and macros in the global namespace Oct 6, 2022
@alranel
Copy link
Member Author

alranel commented Oct 6, 2022

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

#define appendAttributesToCloud() appendAttributesToCloudReal(CborEncoder *encoder)
#define appendAttribute(x) appendAttributeReal(x, getAttributeName(#x, '.'), encoder)
#define setAttribute(x) setAttributeReal(x, getAttributeName(#x, '.'))

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.

@facchinm
Copy link
Contributor

facchinm commented Oct 7, 2022

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?

@alranel
Copy link
Member Author

alranel commented Oct 12, 2022

@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_?

@facchinm
Copy link
Contributor

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

@pennam
Copy link
Collaborator

pennam commented Nov 10, 2022

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

enum class Permission {
Read, Write, ReadWrite
};

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 pennam reopened this Dec 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants