Skip to content
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

Fixed to allow compilation on macOS M1 #49

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

ducha-aiki
Copy link

Full story described here #47

@sarlinpe sarlinpe marked this pull request as draft April 15, 2022 07:35
@Tobias-Fischer
Copy link

We use this pull request in conda-forge (see #93) - is there a reason why it is not being merged?

@Tobias-Fischer
Copy link

I think the changes around FindPythonPyEnv are not needed, but the other ones are relatively trivial and are required for clang.

@ducha-aiki ducha-aiki marked this pull request as ready for review June 30, 2023 05:30
@sarlinpe
Copy link
Member

Thanks @ducha-aiki for the PR. We'll merge only if the changes are minimally required for the feature - it looks like right now there they're fixing multiples issues at once. Are these changes related to M1 architectures or to clang? I don't have an M1 computer at hand but I can run clang.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
pixsfm/base/src/interpolation.h Outdated Show resolved Hide resolved
pixsfm/base/src/interpolation.h Outdated Show resolved Hide resolved
pixsfm/base/src/interpolation.h Outdated Show resolved Hide resolved
pixsfm/base/src/interpolation.h Show resolved Hide resolved
pixsfm/bundle_adjustment/src/reference_extractor.h Outdated Show resolved Hide resolved
third-party/progressbar.h Outdated Show resolved Hide resolved
pixsfm/util/src/memory.h Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@ducha-aiki
Copy link
Author

Well, for me, the "feature" is "works on my M1 machine with minimal changes". I don't have any Intel clang, unfortunately. Let me check if that works w/o FindPythonPyEnv and with auto-AVX

@ducha-aiki ducha-aiki requested a review from sarlinpe July 10, 2023 10:30
@ducha-aiki
Copy link
Author

@skydes I have updated PR

@Tobias-Fischer
Copy link

LGTM :)

// The following two Evaluate overloads are needed for interfacing
// with automatic differentiation. The first is for when a scalar
// evaluation is done, and the second one is for when Jets are used.
template <>
Copy link
Member

Choose a reason for hiding this comment

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

Explicit specialization in class scope is accepted by clang but not by GCC:

error: explicit specialization in non-namespace scope ‘class pixsfm::Interpolator’

This is part of the standard only since C++17 and still a bug in GCC: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=85282 Will look into this when I find the time.

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.

3 participants