This is an archive of the discontinued LLVM Phabricator instance.

Cleanup: move visibility/linkage attributes to the first declaration (part 2).
ClosedPublic

Authored by eugenis on Dec 9 2015, 6:07 PM.

Details

Summary

This is a follow-up to r252385.
For some reason, I missed a lot of cases when the visibility attribute was applied to the definition, but not to an earlier declaration.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 42369.Dec 9 2015, 6:07 PM
eugenis retitled this revision from to Cleanup: move visibility/linkage attributes to the first declaration (part 2)..
eugenis updated this object.
eugenis added reviewers: EricWF, mclow.lists.
eugenis set the repository for this revision to rL LLVM.
eugenis added a subscriber: cfe-commits.
EricWF accepted this revision.Apr 19 2016, 7:55 PM
EricWF edited edge metadata.

LGTM.

test/libcxx/test/config.py
367 ↗(On Diff #42369)

Get rid of this.

This revision is now accepted and ready to land.Apr 19 2016, 7:55 PM

I've updated the diff so it merges and also fixed <ostream>, <istream>, <sstream> and <streambuf>. It can be found here: https://gist.github.com/EricWF/487e5b1de2bb320e93fbb3c9c758b013

It seems my changes to the IO headers removes a bunch of symbols from the dylib. I'll look into it. I'm assuming it's because clang was already ignoring the "_LIBCPP_INLINE_VISIBILITY" attribute when instantiating extern templates. The symbols get removed even if we don't build with "internal_linkage".

So you pointed out that my changes to the streams is not ABI compatible. Obviously that cannot be a part of this patch then.

eugenis updated this revision to Diff 54562.Apr 21 2016, 12:28 PM
eugenis edited edge metadata.

Updates with Eric's patch from
https://gist.github.com/EricWF/487e5b1de2bb320e93fbb3c9c758b013
without the iostream changes.

This change does not affect libc++.so on Linux in any way.

Still LGTM.

eugenis closed this revision.Apr 21 2016, 6:11 PM

r267093
Thanks for your help!