search

arduino / ArduinoCore-API

Hardware independent layer of the Arduino cores defining the official API
https://www.arduino.cc/reference/en/
GNU Lesser General Public License v2.1
216 stars 120 forks source link

Add include guards instead of #pragma once #171

Closed PatrickDekker98 closed 2 years ago

PatrickDekker98 commented 2 years ago

https://github.com/arduino/ArduinoCore-API/blob/e26862e453c1234e1c23506d1839bfa68999d911/api/Udp.h#L35

Pragma once instead of include guards causes error: redefinition of 'class arduino::UDP' with the ArduinoCore-samd.

An easy fix would be to add:

#ifndef udp_h
#define udp_h
....
#endif

Or remove the double class definition

matthijskooijman commented 2 years ago

Any more details on why this happens? Does ArduinoCore-samd define its own version of the Udp class somehow and using the same include guards on both ensures only one of them is added? If so, I think that would be fragile, since depending on include ordering you might get one or the other definition?

PatrickDekker98 commented 2 years ago

https://github.com/arduino/ArduinoCore-samd/blob/master/cores/arduino/Udp.h ArduinoCore-samd defines its own Udp class, which is functionaly the same as the one in core-api except it uses include guards instead of #pragma once

matthijskooijman commented 2 years ago

Sounds like this is really a bug in ArduinoCore-samd and that the Udp.h should be removed from there?

PatrickDekker98 commented 2 years ago

I just checked, core-stm32 and core-avr have the same problem. Of course deleting it in arduino-core would be the most efficient but I am not sure what problems that could cause.

matthijskooijman commented 2 years ago

I just checked, core-stm32 and core-avr have the same problem.

I think those other two cores do not include ArduinoCore-API yet (they just duplicate everything, from before ArduinoCore-API existed).

When you say "the same problem", do you mean you get the same errors? Or just that they contain an Udp.h (since the latter is to be expected)?

Of course deleting it in arduino-core would be the most efficient but I am not sure what problems that could cause.

Seems like that would be the proper solution here, and if the include paths are set up correctly so #include <Udp.h> still works, I would not expect any problems here. Maybe you could report an issue in that repo and close this one?

PatrickDekker98 commented 2 years ago

I think those other two cores do not include ArduinoCore-API yet (they just duplicate everything, from before ArduinoCore-API existed). When you say "the same problem", do you mean you get the same errors? Or just that they contain an Udp.h (since the latter is to be expected)?

They both contain an Udp.h if you say that is to be expected I trust you. I'm not sure what arduino's design decisions where or are. To me It would seem that Udp.h is hardware independent so it should probaply be in Arduino-core. I'll close this issue

matthijskooijman commented 2 years ago

I'm not sure what arduino's design decisions where or are.

It's just that originally ArduinoCore-API did not exist and each core would duplicate all hardware-independent code. Then Arduino-Core-API was invented to house all this independent code, but not all cores have been modified to include ArduinoCore-API yet, so at least the AVR core still contains all of the independent code directly, instead of including ArduinoCore-API, which is why it is expected that Udp.h is in there, but not in ArduinoCore-samd.