Page MenuHomePhabricator

Make methods of exported API classes that access internal types private.
ClosedPublic

Authored by zturner on May 27 2014, 5:46 PM.

Details

Reviewers
tfiala
Summary

As part of the contract of a dll-exported C++ interface, classes that are not exported should not be accessible via a public interface of an exported class, and doing so generates warnings on MSVC. These warnings can be safely suprressed as long as all non-exported classes are part of the exported class's private interface, so this change moves all such methods to the private interface, friending where necessary, and finally suppresses the warnings on MSVC.

Diff Detail

Event Timeline

zturner updated this revision to Diff 9856.May 27 2014, 5:46 PM
zturner retitled this revision from to Make methods of exported API classes that access internal types private..
zturner updated this object.
zturner edited the test plan for this revision. (Show Details)
zturner added a reviewer: tfiala.
zturner added a subscriber: Unknown Object (MLST).
tfiala edited edge metadata.May 28 2014, 11:00 AM

I ran into a few issues with this change on both gcc and clang.

The first issue is the handling of the friend class namespace. Prepending a :: to the friend declaration for non-namespaced implementation classes took care of one of them.

The bigger issue is that the Swig-generated classes are failing in a number of static methods that need access to the now-private methods. After discussing with Zachary, I'm testing just the CMakeLists.txt portion for now and aborting the other portion until he can work on a swig-enabled build.

The CMakeLists.txt cmake change warning suppression was submitted with this commit:

svn commit
Sending CMakeLists.txt
Transmitting file data .
Committed revision 209756.

tfiala added a comment.Jun 5 2014, 7:56 AM

Current state: Zachary is going to look into addressing swig incompatibilities with the non-cmake portion of this change. We're keeping this ticket open until that portion is handled.

Hey Zachary - I'd like to close this one out (or at least get it out of my inbox as a task). Can we break the second part of it into a separate task and call this one done on the first part?

Yes, I had totally forgotten about this one. And now I'm in the weeds on
more pressing issues. So let's call this one done and I'll treat the
remainder as a minor cleanup task, and get back to it when I have some
spare cycles.

tfiala accepted this revision.Jul 22 2014, 9:17 AM
tfiala edited edge metadata.

Good deal :-)

This revision is now accepted and ready to land.Jul 22 2014, 9:17 AM
tfiala closed this revision.Jul 22 2014, 9:17 AM