Skip to content

Conversation

@RaymiiOrg
Copy link
Collaborator

As I saw in the github actions pull request (#26 ), compilation with clang on OS X fails due to not having a make_unique (which is a C++ 14 feature). This is a workaround which should do the same.

@lenticularis39
Copy link
Owner

It's a little ironic that it was me who wanted to target C++11 instead of C++14 and then I break macOS by std::make_unique, which is apparently a C++14 feature (didn't know that before) :D

Anyway the include of make_unique.h should IMO be in StdAfx.hpp (also make_unique.h should be named make_unique.hpp).

Also checking specifically for __APPLE__ doesn't sound right - there may be (now or in the future) a situation where std::make_unique is present. Checking for C++ standard below 14 using #if __cplusplus < 201402L should be better - it may still fail on older compilers that support only a part of C++14 including std::make_unique, though.

@RaymiiOrg RaymiiOrg force-pushed the os_x_cpp11_make_unique branch from e1fdb3c to 37ac94b Compare November 19, 2020 06:54
@RaymiiOrg
Copy link
Collaborator Author

RaymiiOrg commented Nov 19, 2020

It's a little ironic that it was me who wanted to target C++11 instead of C++14 and then I break macOS by std::make_unique, which is apparently a C++14 feature (didn't know that before) :D

Anyway the include of make_unique.h should IMO be in StdAfx.hpp (also make_unique.h should be named make_unique.hpp).

Also checking specifically for __APPLE__ doesn't sound right - there may be (now or in the future) a situation where std::make_unique is present. Checking for C++ standard below 14 using #if __cplusplus < 201402L should be better - it may still fail on older compilers that support only a part of C++14 including std::make_unique, though.

Updated branch with both changes. I agree that it's better to check for the older compiler instead of just apple. Thanks for the #if example!

Apple build now succeeds: https://github.com/RaymiiOrg/axpbox/runs/1422659565?check_suite_focus=true - tests not yet but that is for a different ticket.

@lenticularis39 lenticularis39 merged commit 7ca7af0 into lenticularis39:main Nov 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants