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

Add Visibility API for gcc/clang toolsets #1044

Merged
merged 7 commits into from
Apr 17, 2018

Conversation

tdesveauxPKFX
Copy link
Contributor

No description provided.

Copy link
Member

@samsinsane samsinsane left a comment

Choose a reason for hiding this comment

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

This would be much better implemented as an API instead of a flag. We're avoiding adding new flags, they start off as binary but eventually cease being binary. Also, when implementing as an API please consider Clang.

@tdesveauxPKFX
Copy link
Contributor Author

It seems that Clang copy all cxx flags from gcc here.

I also added the other possible values from what running gcc -fvisibility=x displayed.

And I'm wondering about removing -fvisibility-inlines-hidden from this. It could be misleading in the present state.

@samsinsane
Copy link
Member

Thanks for making those changes! I did some basic reading of what -fvisibility-inlines-hidden does, does it add anything to -fvisibility=hidden? It didn't look like it, but I just skimmed the GCC wiki. I'm happy for this to get merged in, but I'll leave it for someone else to review and add their opinion on -fvisibility-inlines-hidden. Thanks again!

@tdesveauxPKFX
Copy link
Contributor Author

From what I read, it seems indeed that visibility-inlines-hidden can be useful on it's own only.
I will setup an other API for it.

@samsinsane
Copy link
Member

Sounds good!

@tdesveauxPKFX
Copy link
Contributor Author

Inlines visibility moved to another PR (#1049)

@tvandijck tvandijck closed this Apr 12, 2018
@tdesveauxPKFX
Copy link
Contributor Author

@tvandijck why did you close this?
Although I move inlines visibility to another PR, the visibility API is still useful.

@samsinsane
Copy link
Member

I'm going to assume this was an accident.

@samsinsane samsinsane reopened this Apr 12, 2018
@tvandijck
Copy link
Contributor

No, not an accident, just confused.. It said "Inlines visibility moved to another PR (#1049)", so I assumed this one was no longer needed...

At the very least the PR should be renamed to describe what it is now doing though, considering it no longer "Adds VisibilityHidden to flags"...

@tdesveauxPKFX
Copy link
Contributor Author

Indeed, I should have updated the title with the changes I made. My bad.

@tdesveauxPKFX tdesveauxPKFX changed the title Add VisibilityHidden to flags Add Visibility API for gcc/clang toolsets Apr 13, 2018
Copy link
Contributor

@tvandijck tvandijck left a comment

Choose a reason for hiding this comment

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

merge conflict. other then that, ready to merge.

@starkos starkos dismissed tvandijck’s stale review April 17, 2018 16:44

Merge conflict has been cleared.

@starkos starkos merged commit 2dfa956 into premake:master Apr 17, 2018
@tdesveauxPKFX tdesveauxPKFX deleted the visibility-hidden branch April 17, 2018 18:15
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.

4 participants