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 InlinesVisibility API #1049

Merged
merged 1 commit into from
May 7, 2018

Conversation

tdesveauxPKFX
Copy link
Contributor

As said in #1044, inlines visibility should have it's own API.

@TurkeyMan
Copy link
Contributor

Why does this need its own API? Why can't this be expressed with visibility?
It feels very unlikely that you would ever supply -fvisibility-inlines-hidden without also supplying -fvisibility=hidden.

@tdesveauxPKFX
Copy link
Contributor Author

tdesveauxPKFX commented Apr 17, 2018

We looked into it when I submitted #1044 and it seems -fvisibility=hidden "includes" the behavior of -fvisibility-inlines-hidden as far as I know. If you know more about it, I'd be interested.

Otherwise, I opted for a new API as you may want to set -fvisibility-inlines-hidden and let's say -fvisibility=internal.

From gcc documentation

Lastly, there's one other new command line switch: -fvisibility-inlines-hidden. This causes all inlined class member functions to have hidden visibility, causing significant export symbol table size & binary size reductions though not as much as using -fvisibility=hidden. However, -fvisibility-inlines-hidden can be used with no source alterations, unless you need to override it for inlines where address identity is important either for the function itself or any function local static data.

Again, this is not an option I'm that much knowledgeable about so if you have further information about it, I am open to improve this.

@tdesveauxPKFX tdesveauxPKFX force-pushed the visibility-inlines-hidden branch from dc9b9df to 259f118 Compare April 18, 2018 12:15
@tvandijck
Copy link
Contributor

@TurkeyMan do you still have objections? otherwise go ahead and merge this one, LGTM.

@tvandijck tvandijck merged commit a493421 into premake:master May 7, 2018
@tdesveauxPKFX tdesveauxPKFX deleted the visibility-inlines-hidden branch June 1, 2018 20:07
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