Skip to content

[SyntaxText] Always use memcmp #2617

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
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[SyntaxText] Always use memcmp #2617

wants to merge 1 commit into from

Conversation

@rintaro
Copy link
Member

@rintaro rintaro commented Apr 22, 2024

compareMemory(_:_:_:) was to abstract memcmp or Collection.elementsEqual(_:) depending on the platform or build settings. Now that we consistently imports "platform" system module, so memcmp is reliably available.

@rintaro rintaro mentioned this pull request Apr 22, 2024
Draft
@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

Update for https://github.com/apple/swift-syntax/pull/2598/files#r1575312552

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

@swift-ci Please test

rintaro
rintaro commented Apr 22, 2024
View reviewed changes
Sources/SwiftSyntax/SyntaxText.swift
#elseif canImport(Glibc)
import Glibc
#elseif canImport(Musl)
import Musl
Copy link
Member Author

@rintaro rintaro Apr 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In case anyone is wondering if Musl has memcmp: http://git.musl-libc.org/cgit/musl/tree/include/string.h#n30 http://git.musl-libc.org/cgit/musl/tree/src/string/memcmp.c

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

🤔 https://ci.swift.org/job/swift-syntax-PR-macOS/4092/

/Applications/Xcode-beta.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX14.2.sdk/usr/lib/swift/Swift.swiftmodule/x86_64-apple-macos.swiftinterface:13:64: error: unexpected code '...' in parameter clause
   11 |   return y < x ? y : x
   12 | }
   13 | @inlinable public func min<T>(_ x: T, _ y: T, _ z: T, _ rest: T...) -> T where T : Swift.Comparable {
      |                                                                `- error: unexpected code '...' in parameter clause
   14 |   var minValue = min(min(x, y), z)
   15 |   // In case `value == minValue`, we pick `minValue`. See min(_:_:).

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

@swift-ci Please test macOS

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 22, 2024

@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Apr 23, 2024 •
edited
Loading

Windows failure is probably because ucrt.modulemap is not a part of test (i.e. check-swift) dependencies in swift repo.

@rintaro
Copy link
Member Author

rintaro commented Apr 23, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

@swift-ci Please test Windows

@rintaro
[SyntaxText] Always use memcmp
89d663e 
`compareMemory(_:_:_:)` was to abstract `memcmp` or
`Collection.elementsEqual(_:)` depending on the platform or build
settings. Now that we consistently imports "platform" system module, so
`memcmp` is reliably available.
@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

swiftlang/swift#73224
@swift-ci Please test

@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

swiftlang/swift#73224
@swift-ci Please test Windows

2 similar comments
@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

swiftlang/swift#73224
@swift-ci Please test Windows

@rintaro
Copy link
Member Author

rintaro commented Apr 24, 2024

swiftlang/swift#73224
@swift-ci Please test Windows

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ahoppen ahoppen ahoppen approved these changes

@bnbarham bnbarham Awaiting requested review from bnbarham bnbarham is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rintaro @ahoppen