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

cmake: Drop extraneous -fvisibility-inlines-hidden #303

Merged
merged 1 commit into from
Aug 5, 2024

Conversation

hebasto
Copy link
Owner

@hebasto hebasto commented Aug 5, 2024

This PR mirrors the master branch behavior and addresses bitcoin#30454 (comment).

Mirror the master branch behavior.
@hebasto hebasto added the bug Something isn't working label Aug 5, 2024
@m3dwards
Copy link

m3dwards commented Aug 5, 2024

ACK 4dd3928

Straightforward enough. Test on Aarch Mac.

@hebasto hebasto merged commit 2feccd4 into cmake-staging Aug 5, 2024
40 checks passed
@fanquake
Copy link

fanquake commented Aug 5, 2024

How did this end up getting added in the first place?

@hebasto
Copy link
Owner Author

hebasto commented Aug 5, 2024

How did this end up getting added in the first place?

It was added in #32.

@fanquake
Copy link

fanquake commented Aug 5, 2024

It was added in #32.

Right, my question was why? It's odd that if it was done on purpose, it'd just be removed with 0 explanation, because if there was a reason, then a PR to master with the same justification would be made. Otherwise, I was wondering why it was added.

@hebasto
Copy link
Owner Author

hebasto commented Aug 5, 2024

It was added in #32.

Right, my question was why?

I don't remember almost a year old staff.

It's odd that if it was done on purpose, it'd just be removed with 0 explanation, because if there was a reason, then a PR to master with the same justification would be made. Otherwise, I was wondering why it was added.

Whatever the initial purpose was, "to mirror the master branch behavior" sounds like an explanation for removal, no?

@theuni
Copy link

theuni commented Aug 5, 2024

It was probably added because the two visibility options get set together: both on or both off. @hebasto probably just assumed that logic at the time.

Might be worth a PR to add it to master rather than removing it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants