Skip to content

[stdlib] Remove #warning in Random.cpp (_WIN32) #20414

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

Closed
wants to merge 1 commit into from

Conversation

benrimmington
Copy link
Contributor

No description provided.

@benrimmington
Copy link
Contributor Author

@compnerd Have you managed to test swift_stdlib_random on Windows?

Please merge this pull request, if you think the #warning isn't needed.

@compnerd
Copy link
Member

compnerd commented Nov 8, 2018

@benrimmington no, I don't think that we have tests for this yet which is why I haven't removed the warning yet.

@benrimmington
Copy link
Contributor Author

swift_stdlib_random is used in SystemRandomNumberGenerator._fill(bytes:), which has a validation-test.

Should the _fill(bytes:) customization point be removed in Swift 5? It doesn't appear to be used elsewhere (since #17604).

Could the validation-test call swift_stdlib_random (exported via SWIFT_RUNTIME_STDLIB_API) directly?

Cc: @mikeash, @Azoy

@Azoy
Copy link
Contributor

Azoy commented Nov 8, 2018

@benrimmington I have a ready implementation to formally introduce fillBytes(_:), but I haven't gotten around to writing the actual proposal. I'm not sure even if I had something ready by tomorrow that it'd go through review before Nov. 16 however. I've been very busy 😞

@benrimmington
Copy link
Contributor Author

@Azoy Can your fillBytes(_:) proposal be reviewed for Swift 5.1 or later? It might be able to use the new DataProtocol.

Does your next(upperBound:) amendment need to be reviewed by next week?

Shall I remove _fill(bytes:) now, so that it isn't in the Swift 5.0 ABI?

Shall I also remove the _fill(bytes:) validation-test? Other tests are indirectly using swift_stdlib_random.

@mikeash
Copy link
Contributor

mikeash commented Nov 8, 2018

If it's not used and has an underscore prefix then it seems like _fill(bytes:) is just an attractive nuisance and it would be best to remove it.

@Azoy
Copy link
Contributor

Azoy commented Nov 8, 2018

cc: @lorentey who might have some opinions on what we could do.

@Azoy
Copy link
Contributor

Azoy commented Nov 9, 2018

cc: @airspeedswift as well

@lorentey
Copy link
Member

lorentey commented Nov 9, 2018

Oh, I haven't realized we aren't using _fill(bytes:) anymore. Removing it seems like a good idea indeed!

As long as we don't need it, It seems cleaner to add a de-underscored version later than trying to support both.

@lorentey
Copy link
Member

lorentey commented Nov 9, 2018

I submitted #20463 to remove it!

@benrimmington benrimmington deleted the random-warning branch November 9, 2018 23:36
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