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

Fix Swift 5.9 warning #4687

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

Conversation

svobs
Copy link
Contributor

@svobs svobs commented Nov 11, 2023


Description:

See comments below

Copy link
Member

@saagarjha saagarjha left a comment

Choose a reason for hiding this comment

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

Don't merge this yet. The changes here should be as follows:

  1. Constrain to FixedWidthInteger.
  2. Add an overload for Double.
  3. Figure out what we need to do to serialize timespec correctly and in a backwards-compatible manner.

When BitwiseCopyable is available, we should migrate to that.

@svobs
Copy link
Contributor Author

svobs commented Nov 14, 2023

@saagarjha alright, put a little bit more effort into it...

The documentation for MemoryLayout.size is terrible, but via debugging it was clear that MemoryLayout.size(ofValue: x) was returning incorrect values (it was always 8), but MemoryLayout<T>.size gave correct values.

Not sure if I got your whole point in (3). Looks like timespec is just 2 Int-type values, and although I couldn't find the def for __darwin_time_t, the result of MemoryLayout<timespec>.size is 16 which seems to vouch for that idea.

Didn't dive in to the written files with a hex editor; just did spot testing. Thumbnail generation & Date Last Opened updates seemed to work fine before this, and seem to work fine after.

@saagarjha
Copy link
Member

What value were you trying this on? The two should be consistent in their results. The problem with serializing timespec is that it can include padding if you write it out directly, which something that you generally don't want to do.

@svobs
Copy link
Contributor Author

svobs commented Nov 15, 2023

What value were you trying this on? The two should be consistent in their results.

Well...I guess I was wrong... I could have sworn I saw the debugger showing 8 for both version (UInt8) and timespec. But just tried again and I see that they are consistent. So then everything seems to be equivalent to what it was before.

Did not learn until now that Apple's ARM is little-endian. And found here that __darwin_time_t is expected to be 64-bit also for all 64-bit processors.

@svobs svobs closed this Nov 15, 2023
@svobs svobs reopened this Nov 15, 2023
@svobs
Copy link
Contributor Author

svobs commented May 29, 2024

Pinging @saagarjha: this is still marked as "changes requested" but should be ready to go. Please let me know of any concerns which are blocking this.

@saagarjha
Copy link
Member

Well my point was we should serialize timespec's members and not just try to splat its contents directly.

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.

None yet

2 participants