Skip to content

Overly-generic defines in utils.h break some compilations #389

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

Closed
matthijskooijman opened this issue Dec 13, 2018 · 3 comments
Closed

Overly-generic defines in utils.h break some compilations #389

matthijskooijman opened this issue Dec 13, 2018 · 3 comments
Assignees
Labels
bug 🐛 Something isn't working
Milestone

Comments

@matthijskooijman
Copy link
Contributor

This core contains a few very generic defines in utils.h:

// Concatenate 2 strings
#define CONCAT(s1, s2) (s1 s2)
// Concatenate 2 strings separated by space
#define CONCATS(s1, s2) (s1" " s2)
// Stringification
#define xstr(s) str(s)
#define str(s) #s

Especially the str and xstr defines are problematic, since you might be using those as function or method names, which triggers the preprocessor. I ran into this issue when using the boost library, which has a str method somewhere, but this can be easily reproduced with this sketch:

void str() { }
void setup() { }
void loop() { }

Which gives:

In file included from /home/matthijs/.arduino15/packages/STM32/hardware/stm32/1.4.0/cores/arduino/wiring.h:31:0,
                 from /home/matthijs/.arduino15/packages/STM32/hardware/stm32/1.4.0/cores/arduino/Arduino.h:32,
                 from /tmp/arduino_build_gdfirmware/sketch/sketch_dec13a.ino.cpp:1:
sketch_dec13a:1:10: error: expected unqualified-id before string constant
 void str() { }
          ^
/home/matthijs/.arduino15/packages/STM32/hardware/stm32/1.4.0/cores/arduino/utils.h:13:17: note: in definition of macro 'str'
 #define str(s) #s

Here, it points to the problematic macro, but in my original problem case the second message was not present, making this particularly tricky to diagnose.

As for resolving this:

  • A grep suggests that the str() and xstr() macros are not actually used in the entire core, so I suggest removing them.
  • The CONCAT and CONCATS macros could perhaps be rename as well, but better would be to not expose them from header files. grep shows that these macros are only used in .c files, so simply including utils.h from those .c files, and dropping the include from wiring.h sounds like the best approach to me.
@fpistm fpistm self-assigned this Dec 16, 2018
@fpistm fpistm added this to the 1.5.0 milestone Dec 16, 2018
@fpistm fpistm added the bug 🐛 Something isn't working label Dec 16, 2018
@fpistm
Copy link
Member

fpistm commented Dec 17, 2018

Those can be used at sketch level, that's why utils.h is provided thanks the wiring.h.
This also provide dtostrf function.
I propose to uppercase the str() and xstr() are they are macro.

@fpistm fpistm added the waiting feedback Further information is required label Dec 17, 2018
@matthijskooijman
Copy link
Contributor Author

I propose to uppercase the str() and xstr() are they are macro.

That would only help to some degree, but even in uppercase the name is very generic and chances of collision remain. Uppercasing would reduce the chance of collision, but it is still likely that a sketch will define a STR() macro itself (possibly for the same purpose). A sketch can easily fix this by using a different name, but sketches which were written for other cores would then suddenly fail on STM32.

Those can be used at sketch level, that's why utils.h is provided thanks the wiring.h.

They can be used, but should they? Are they actually part of the core STM32 core API? Are they documented anywhere? The official Arduino cores do not provide them AFAIK, so this would be an STM32-specific addition. Also, I would expect that anyone toying with macros that need this macro, could just as well add these macros themselves. Portable sketches (that run on non-STM32-cores) will have to do so anyway, and if the availability of this macro is not documented (and thus not guaranteed to remain in future version), I would personally not want to rely on it and implement my own version anyway.

This also provide dtostrf function.

That does seem useful, as that seems to be part of the official Arduino API (or at least the de facto standard). But that is of course a matter of including dtostrf.h directly in wiring.h or elsewhere.

@fpistm
Copy link
Member

fpistm commented Dec 17, 2018

A sketch can easily fix this by using a different name, but sketches which were written for other cores would then suddenly fail on STM32.

More than one year this is included without any complains so I guess this is not really used at user sketch. This is only provide for convenience.
CONCATS is used for USB String. Other are currently not used (at least officially).

Are they documented anywhere?

Probably not as I'm late in writing the doc :(

fpistm added a commit to fpistm/Arduino_Core_STM32 that referenced this issue Dec 17, 2018
Include utils.h manually will provide them
Note: str() and xstr() are now capitalized.


Fix stm32duino#389

Signed-off-by: Frederic.Pillon <[email protected]>
fpistm added a commit to stm32duino/STM32Examples that referenced this issue Dec 20, 2018
To avoid any name conflict xstr macro was capitalized and required to
include header file manually.
See stm32duino/Arduino_Core_STM32#389

Signed-off-by: Frederic.Pillon <[email protected]>
@fpistm fpistm removed the waiting feedback Further information is required label Jan 11, 2019
benwaffle pushed a commit to benwaffle/Arduino_Core_STM32 that referenced this issue Apr 10, 2019
Include utils.h manually will provide them
Note: str() and xstr() are now capitalized.


Fix stm32duino#389

Signed-off-by: Frederic.Pillon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants