Skip to content

Conversation

@MaxDesiatov
Copy link
Member

Since Musl is sufficiently different from Glibc (see https://wiki.musl-libc.org/functional-differences-from-glibc.html), it requires a different import, which now should be applied to files that have import Glibc in them.

Since Musl is sufficiently different from Glibc (see https://wiki.musl-libc.org/functional-differences-from-glibc.html), it requires a different import, which now should be applied to files that have `import Glibc` in them.

#if !os(Windows)
#if canImport(Musl)
internal var _O_ACCMODE: CInt { 03|O_SEARCH }
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied verbatim from the header since ClangImport doesn't support C macros that expand to expressions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck. The original 03 is in octal. It so happens that 038 and 0310 have the same value, but that's pretty horrid. I think I would prefer that if we're doing to write this in Swift code, we explicitly write 0o03 or 0x03 rather than just 03, because the latter looks like octal if you're used to C family languages, but in Swift it is in fact decimal with a leading zero.

@MaxDesiatov
Copy link
Member Author

@swift-ci test

#if os(macOS) || os(iOS) || os(watchOS) || os(tvOS)
import Darwin
#elseif os(Linux) || os(FreeBSD) || os(OpenBSD) || os(Android)
#elseif canImport(Glibc)
Copy link
Contributor

@glessard glessard Jun 22, 2023

Choose a reason for hiding this comment

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

This is a big switch from using the OS name to using a sort of capability. Would it make sense to move this to after the #elseif os(Windows) branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

We should do the same in the other files where there's a sequence of if-os and if-canImport.

@MaxDesiatov MaxDesiatov requested a review from glessard June 22, 2023 21:37
@MaxDesiatov
Copy link
Member Author

@swift-ci test

#elseif canImport(Glibc)
import Glibc
#elseif canImport(Musl)
import CSystem
Copy link
Member

Choose a reason for hiding this comment

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

in other spots it is imported with @_implementationOnly, also, do we need this in the Glibc case as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure about this one, I'd stick to how other imports are done in this file for consistency.

@_implementationOnly import CSystem
import Glibc
#elseif canImport(Musl)
import CSystem
Copy link
Member

Choose a reason for hiding this comment

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

@_implementationOnly import CSystem?

#elseif os(Windows)
import CSystem
import ucrt
#elseif canImport(Glibc)
Copy link
Member

Choose a reason for hiding this comment

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

much better!

@MaxDesiatov MaxDesiatov requested a review from tomerd June 22, 2023 23:47
@MaxDesiatov
Copy link
Member Author

@swift-ci test


#if !os(Windows)
#if canImport(Musl)
internal var _O_ACCMODE: CInt { 03|O_SEARCH }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yuck. The original 03 is in octal. It so happens that 038 and 0310 have the same value, but that's pretty horrid. I think I would prefer that if we're doing to write this in Swift code, we explicitly write 0o03 or 0x03 rather than just 03, because the latter looks like octal if you're used to C family languages, but in Swift it is in fact decimal with a leading zero.

@MaxDesiatov
Copy link
Member Author

@swift-ci test

@MaxDesiatov MaxDesiatov merged commit 0062c82 into main Jul 4, 2023
@MaxDesiatov MaxDesiatov deleted the maxd/canimport-musl branch July 4, 2023 15:50
@glessard glessard added this to the 1.3.0 milestone Aug 17, 2023
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.

5 participants