-
Notifications
You must be signed in to change notification settings - Fork 513
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
Handle differences in math headers compiling with Clang #2352
base: develop
Are you sure you want to change the base?
Conversation
@@ -24,6 +24,11 @@ | |||
****************************************************************************** | |||
*/ | |||
|
|||
#ifdef __clang__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not need to be __clang__
-specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
user/inc/Arduino.h
Outdated
@@ -193,6 +193,9 @@ typedef volatile uint32_t RwReg; | |||
// C++ only | |||
#ifdef __cplusplus | |||
|
|||
#ifndef __clang__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest also not using targeting __clang__
specifically and instead include <cmath>
if compiling as __cplusplus
and math.h
otherwise, which should resolve the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. Note that my fix still includes math.h
in all cases since I think that's required for backwards compatibility with user code as described in the new comment. My C experience is limited so I'm prepared to be wrong about that!
I also chose to put the conditional include of <cmath>
adjacent to the include of math.h
since I think it's clearer even though it requires a separate #ifndef __cplusplus
guard.
user/inc/Arduino.h
Outdated
// In order to support compiling with Clang, explicitly include cmath here since the Clang version | ||
// of the math.h header included above does not include cmath. This is a no-op compiling with GCC | ||
// since the GCC version of math.h includes cmath. Replacing the math.h include above | ||
#include <cmath> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is now included twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gah, sorry, I caught this locally but forgot to push. Fixed now.
Problem
Attempting to compile for unit testing using Clang fails due to differences in the math headers with the errors below.
Solution
Add clang-specific preprocessor directives to handle these cases. There appears to be precedent for clang-specific directives in this project:
https://github.com/particle-iot/device-os/search?q=__clang__
Steps to Test
I wrote a demo project to use Bazel and Google Test with Particle for unit testing. To make it compile successfully, I had to include the changes from this PR as a patch:
https://github.com/metcalf/arduino-particle-bazel-demo/blob/main/tools/io_particle_device_os.patch
Example App
N/A
References
Links to the Community, Docs, Other Issues, etc..
Completeness