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

common_defs.pxd: remove inline modifier #105

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

flokli
Copy link
Contributor

@flokli flokli commented Aug 17, 2019

This causes compilation errors with Clang, for example when building on MacOS.

See https://grokbase.com/t/gg/cython-users/13987sentw/cython-inline-causing-errors-on-macos-with-clang#20130908e6otbth2q2uhx5hqe7ujqsrxe4

I successfully ran the testsuite on it after applying the change, both on x86_64-linux and MacOS.

@bmcage
Copy link
Owner

bmcage commented Aug 23, 2019

@aragilar James, will this not give a slowdown? I assume not really as this is a single action, the for loop internally of these functions is the slow one and that is inlined.

I do note however that clang only gives these errors for the inline that have except? -1 statements, the others seem to be not a problem. Because of this we have cdef inline int and not cdef inline void, even though on success no return value is given. Does this link with #99 ?

Anyway, I believe we can drop these 4 inline's as in the patch without real influence on speed. Agree?

@flokli The inline should in the patch also be removed in common_defs.pyx , so this file matches the pxd sigs you changed. Please do that so that tests run with the full changesert.

@flokli
Copy link
Contributor Author

flokli commented Aug 25, 2019

@bmcage I also changed lines in common_defs.pyx, thanks.

@aragilar
Copy link
Collaborator

From https://clang.llvm.org/compatibility.html#inline, it sounds like inline doesn't actually guarantee the function gets inlined (either in gcc or clang), so dropping inline seems like the best thing to do actually (and we can check whether it's actually been inlined via the -Winline flag as per https://stackoverflow.com/questions/5223690/cs-inline-how-strong-a-hint-is-it-for-gcc-and-clang-llvm).

@bmcage bmcage merged commit 93075ae into bmcage:master Aug 29, 2019
@flokli flokli deleted the common-defs-remove-inline branch August 29, 2019 13:55
@flokli
Copy link
Contributor Author

flokli commented Aug 29, 2019

Thanks!

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