Skip to content

Fix accidentally-introduced warnings and regression in test behavior in PIFLoadingTests #135

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

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

jakepetroules
Copy link
Collaborator

Some of the conditional checks had redundant uses of #require, and regressed previous behavior by failing to include an else case to induce a test failure when the condition was not satisfied.

…in PIFLoadingTests

Some of the conditional checks had redundant uses of #require, and regressed previous behavior by failing to include an else case to induce a test failure when the condition was not satisfied.
@jakepetroules
Copy link
Collaborator Author

@swift-ci test

@jakepetroules jakepetroules enabled auto-merge (rebase) February 10, 2025 06:10
@jakepetroules jakepetroules merged commit a6a6bb7 into main Feb 10, 2025
1 of 2 checks passed
@jakepetroules jakepetroules deleted the fix-warnings-and-test-regressions branch February 10, 2025 06:25
@grynspan
Copy link

There is no need for an else case though. An issue is recorded by #require on failure even if the error is caught.

@grynspan
Copy link

(And the warnings were compiler bugs. You missed the discussion I think?)

@mhrawdon
Copy link
Contributor

Right, as @grynspan says, these changes aren't necessary:

  1. #require will already generate an issue in addition to throwing, which is documented.
  2. The warnings emitted by this use are a compiler bug, tracked here, but we shouldn't work around non-blocking compiler bugs just to sidestep some temporary errors.

We should undo these changes as the old version was more compact and ergonomic. I'll put up a PR.

@mhrawdon
Copy link
Contributor

Reverting in #138

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.

3 participants