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.

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

Jump to bottom

Need better platform code separation #3

Closed
Myriachan opened this issue Feb 7, 2018 · 5 comments · Fixed by #194
Closed

Need better platform code separation #3

Myriachan opened this issue Feb 7, 2018 · 5 comments · Fixed by #194
Assignees
@gchatelet
Labels
enhancement New feature or request
Milestone
v0.7.0

Comments

@Myriachan
Copy link

I was interested in adding iOS, macOS and Windows support, but the layout of the code is not conducive to this. src/cpuinfo_aarch64.c is highly Linux-specific. For an ARM64 Windows implementation, where would the code go? (In Windows, the ARM64 implementation would be based around IsProcessorFeaturePresent.)
@gchatelet
Copy link
Collaborator

Thank you very much for this feedback, I very much appreciate it.
I agree the code layout must be more principled to allow the matrix of OS/ARCH to be fully exposed.

How about cpuinfo_{arch}_{OS}.c? Any suggestion?

@arter97
Copy link

arter97 commented Feb 8, 2018

I'd personally do separate directory approach.
I'd vote on {OS}/{arch}.c.

@f1nalspace
Copy link

I vote on the suggestion from gchatelet.

@Androbin
Copy link

Androbin commented Feb 9, 2018

Using separate directories would allow for each OS to share code using additional files, still isolated from the rest.

@gchatelet
Copy link
Collaborator

@Androbin 's rationale makes sense. I'll play a bit with the layout and come back with a proposal.

@gchatelet gchatelet added the enhancement New feature or request label Feb 12, 2018
@gchatelet gchatelet self-assigned this Mar 13, 2018
@gchatelet gchatelet mentioned this issue Apr 19, 2018
Closed
gchatelet added a commit that referenced this issue Jan 17, 2019
@gchatelet
# This is a combination of 13 commits.
abf08af 
# This is the 1st commit message:

Update macros to detect mips64 and differentiate between x86 32/64.

# This is the commit message #2:

Fix Mips32 and add an alias for Mips32/64.

# This is the commit message #3:

Allow specifying cmake generator.

# This is the commit message #4:

Remove unavailable MSVC version and test Ninja on travis

# This is the commit message #5:

Add ninja build.

# This is the commit message #6:

Use Ninja by default

# This is the commit message #7:

Show tests output

# This is the commit message #8:

Defaulting env to Ninja in the build matrix

# This is the commit message #9:

Defaulting env to Ninja in the build matrix

# This is the commit message #10:

Do not use ninja on OSX

# This is the commit message #11:

Fix test output.

# This is the commit message #12:

Fix running tests

# This is the commit message #13:

Fix running tests
@gchatelet gchatelet linked a pull request Oct 27, 2021 that will close this issue
New code layout - breaking change in cpu_features_macros.h #194
Merged
@gchatelet gchatelet mentioned this issue Oct 28, 2021
Merged
gchatelet added a commit that referenced this issue Oct 28, 2021
@gchatelet
New code layout - breaking change in cpu_features_macros.h (#194)
deb2a61 
This commit helps with platform code separation (fixes #3). It should also help with the build as we can simply include all `impl_*.c` files regardless of OS / arch.

Note: this patch contains breaking changes in `include/cpu_features_macros.h`
 - `CPU_FEATURES_OS_LINUX_OR_ANDROID` does not exist anymore
 - `CPU_FEATURES_OS_FREEBSD`, `CPU_FEATURES_OS_ANDROID` and `CPU_FEATURES_OS_LINUX` are now mutually exclusive (i.e. `CPU_FEATURES_OS_ANDROID` does not imply `CPU_FEATURES_OS_LINUX`)
 - `CPU_FEATURES_OS_DARWIN` has been renamed into `CPU_FEATURES_OS_MACOS` to be able to target non-Mac Apple products (IOS, TV, WATCH). They are now targetable with `CPU_FEATURES_OS_IPHONE`. This matches Apple naming convention described in [this stackoverflow](https://stackoverflow.com/a/49560690).
@Mizux Mizux added this to the v0.7.0 milestone Nov 16, 2021
jwatte pushed a commit to jwatte/cpu_features that referenced this issue Dec 22, 2021
Fix the build for M1 MacBooks
8715830 
It turns out, MACOS is a perfectly fine OS to support aarch64 (through its name
arm64.)

The only thing missing was the appropriate check in impl_aarch64, plus renaming
the file to match that it's really for any OS that has the CPU.

jwatte@Jons-MBP build % make test
Running tests...
Test project /Users/jwatte/cpu_features/build
    Start 1: bit_utils_test
1/4 Test google#1: bit_utils_test ...................   Passed    0.06 sec
    Start 2: string_view_test
2/4 Test google#2: string_view_test .................   Passed    0.04 sec
    Start 3: stack_line_reader_test
3/4 Test google#3: stack_line_reader_test ...........   Passed    0.04 sec
    Start 4: cpuinfo_aarch64_test
4/4 Test google#4: cpuinfo_aarch64_test .............   Passed    0.12 sec
@jwatte jwatte mentioned this issue Dec 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gchatelet gchatelet

Labels
enhancement New feature or request
Projects
None yet
Milestone
v0.7.0
Development

Successfully merging a pull request may close this issue.

New code layout - breaking change in cpu_features_macros.h google/cpu_features
6 participants
@gchatelet @Mizux @arter97 @Myriachan @f1nalspace @Androbin