Skip to content

[ClangImporter] Adjust tests after Clang changes for __attribute__((pure)) #75055

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

egorzhdan
Copy link
Contributor

There was a recent change in Clang's handling of __attribute__((pure)): llvm/llvm-project#78200

__attribute__((pure)) is now dropped for const functions, and for functions that return void.

This adjusts the test for the new behavior. There might be future changes required in the Swift's handling of __attribute__((pure)).

rdar://131300381

…pure))`

There was a recent change in Clang's handling of `__attribute__((pure))`: llvm/llvm-project#78200

`__attribute__((pure))` is now dropped for const functions, and for functions that return `void`.

This adjusts the test for the new behavior. There might be future changes required in the Swift's handling of `__attribute__((pure))`.

rdar://131300381
@egorzhdan egorzhdan requested a review from beccadax July 8, 2024 12:16
@egorzhdan egorzhdan requested review from zoecarver and hyp as code owners July 8, 2024 12:16
@egorzhdan
Copy link
Contributor Author

@swift-ci please test

@egorzhdan egorzhdan requested review from Xazax-hun and ahatanaka July 8, 2024 12:45
@@ -54,7 +54,7 @@ struct Base {
return i * 2;
}

void pure() const __attribute__((pure)) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the const might not be a problem here. I think the const in the clang change might refer to the attribute: https://clang.llvm.org/docs/AttributeReference.html#const

That being said, I am also OK leaving the PR as is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, you're right! Let me bring back const.

Copy link
Contributor

@Xazax-hun Xazax-hun left a comment

Choose a reason for hiding this comment

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

LGTM!

@egorzhdan
Copy link
Contributor Author

Closing this in favor of #75058.

@egorzhdan egorzhdan closed this Jul 8, 2024
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.

2 participants