This is an archive of the discontinued LLVM Phabricator instance.

Cleanup: move all visibility attributes to the first declaration.
ClosedPublic

Authored by eugenis on Nov 5 2015, 6:24 PM.

Details

Summary

This change moves visibility attributes from out-of-class method definitions to in-class declaration.
This is needed for a switch to attribute((internal_linkage)) (see http://reviews.llvm.org/D13925) which can only appear on the first declaration.

This change does not touch istream/ostream/streambuf, which are handled separately in http://reviews.llvm.org/D14409.

Diff Detail

Repository
rL LLVM

Event Timeline

eugenis updated this revision to Diff 39453.Nov 5 2015, 6:24 PM
eugenis retitled this revision from to Cleanup: move all visibility attributes to the first declaration..
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 edited edge metadata.Nov 5 2015, 10:34 PM

Thank you so much! I was procrastination taking this on.

EricWF added a comment.Nov 6 2015, 1:54 PM

Is this a mechanical change, or do you remove/add some attributes in this patch?

This is a mechanical change, and AFAIR I checked that no bits of libc++.so are affected.

The same mechanical change in streambuf/istream/ostream (the stuff that's parts of extern templates) breaks libc++ because of https://llvm.org/bugs/show_bug.cgi?id=25427, that's why it's not included here.

I'll verify that this is not a functional change one more time.

EricWF accepted this revision.Nov 6 2015, 2:06 PM
EricWF edited edge metadata.

It's not just libc++.so that could have it's ABI affected by adding/removing these. It's any shared library build against libc++. However if we are just moving the attribute to the proper place that should truly NFC modolo https://llvm.org/bugs/show_bug.cgi?id=25427.

LGTM and feel free to commit any similar cleanup without review. I'll have to think a little harder about the extern template issues before working through those.

This revision is now accepted and ready to land.Nov 6 2015, 2:06 PM

Yes. I'm just using libc++.so as a (incomplete) test that this is actually NFC. It did catch the extern template issue.

eugenis closed this revision.Nov 6 2015, 5:25 PM

Confirmed and landed as r252385.
Thanks for the quick response!