Skip to content

Handle differences in math headers compiling with Clang #2352

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.

Sign up for GitHub

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

Open
metcalf wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Handle differences in math headers compiling with Clang #2352

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5d907db
Handle differences in math headers compiling with Clang
andrew-stripe Sep 7, 2021
4b850fd
Respond to PR comments avoiding Clang-specific directives
andrew-stripe Sep 7, 2021
f82133a
Remove duplicate include
andrew-stripe Sep 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions user/inc/Arduino.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,9 @@ typedef volatile uint32_t RwReg;
// C++ only
#ifdef __cplusplus

#ifndef __clang__

@avtolstoy avtolstoy Sep 7, 2021

Copy link
Copy Markdown
Member

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.

@metcalf metcalf Sep 7, 2021

Copy link
Copy Markdown
Author

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.

// Under clang, "math.h" does not put isnan/isinf inside the std namespace so this would fail

#ifndef isnan
using std::isnan;
#endif
Expand All @@ -201,6 +204,7 @@ using std::isnan;
using std::isinf;
#endif

#endif // __clang__

// Hardware serial defines

Expand Down
5 changes: 5 additions & 0 deletions wiring/src/spark_wiring_usbserial.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,11 @@
******************************************************************************
*/

#ifdef __clang__

@avtolstoy avtolstoy Sep 7, 2021

Copy link
Copy Markdown
Member

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.

@metcalf metcalf Sep 7, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

// Needed for std::{min,max} in Clang
#include <algorithm>
#endif

#include "spark_wiring_usbserial.h"
#include "platform_headers.h"

Expand Down