Intrusive 'private' and 'protected' macros (cleanup list)

[davidlt]

We cannot redefine 'private' and 'protected' keywords via macros to e.g.
'public'. This is extremely intrusive and breaks encapsulation.

This does not work anymore with new libstdc++ libraries, because foward
delcaration of struct is implicitly private and then implementation is
under explicit private clause. Redefining 'private' only change one of
them thus creating compile-time errors in sstream.

Details in PR65899 (GCC BZ). It's WONTFIX.

Such cleanups are required for GCC 5 and above.

The files in this PR used such intrusive macros, but removal of them
breaks compilation. Thus additional review and refactoring is required
for droping intrusive macros. This PR acts as a cleanup list.

Most of such things (about 35 out of 45) are used in tests.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

[cmsbuild]

A new Pull Request was created by @davidlt for CMSSW_7_6_X.

Intrusive 'private' and 'protected' macros (cleanup list)

It involves the following packages:

CalibTracker/SiStripDCS
CondCore/DBCommon
CondFormats/Common
DataFormats/Common
DataFormats/EcalDigi
DataFormats/Provenance
DataFormats/TrackReco
FWCore/Framework
FWCore/PluginManager
FWCore/ServiceRegistry
FWCore/Utilities
Fireworks/Core
Fireworks/Electrons
Fireworks/Eve
Fireworks/FWInterface
Fireworks/Geometry
Fireworks/Vertices
MagneticField/Engine
MagneticField/GeomBuilder
MuonAnalysis/MomentumScaleCalibration
PerfTools/Callgrind
RecoPixelVertexing/PixelLowPtUtilities
RecoPixelVertexing/PixelTriplets
SimG4Core/Geometry
TrackingTools/PatternTools

@smuzaffar, @civanch, @diguida, @cvuosalo, @mdhildreth, @monttj, @cmsbuild, @alja, @franzoni, @Dr15Jones, @cerminar, @slava77, @ggovi, @vadler, @mmusich can you please review it and eventually sign? Thanks.
@ghellwig, @abbiendi, @argiro, @Martin-Grunewald, @frmeier, @battibass, @makortel, @jhgoh, @jlagram, @cerati, @trocino, @wmtan, @GiacomoSguazzoni, @namapane, @rovere, @VinInn, @bellan, @mmusich, @dgulhan, @apfeiffer1, @gbenelli, @wddgit, @alja, @gpetruc, @istaslis this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.
@Degano you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

[slava77]

@cmsbuild please test

[davidlt]

@slava77 this PR will not compile. It only includes cleanups which do not compile. These require a review and refactoring to work without redefining 'private' and 'public'.

[cmsbuild]

The tests are being triggered in jenkins.

[slava77]

@davidlt
ah, ok, I didn't read the PR message carefully. Well, at least we'll get the list of errors then.

[VinInn]

I suggest to use sed instead of the macro...
after all, why should we use private/protected at all?

[cmsbuild]

-1
Tested at: e16ef63
I found an error when building:

>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-27-2300/src/Fireworks/Core/src/FWExpressionEvaluator.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-27-2300/src/Fireworks/Core/src/FWExpressionValidator.cc 
>> Compiling  /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-27-2300/src/Fireworks/Core/src/FWFileEntry.cc 
In file included from /tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-27-2300/src/Fireworks/Core/src/FWEveView.cc:22:0:
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/lcg/root/6.02.12-kpegke/include/TGLOrthoCamera.h: In member function 'void FWEveView::addToOrthoCamera(TGLOrthoCamera*, FWConfiguration&) const':
/afs/cern.ch/cms/sw/ReleaseCandidates/vol0/slc6_amd64_gcc493/lcg/root/6.02.12-kpegke/include/TGLOrthoCamera.h:54:19: error: 'Double_t TGLOrthoCamera::fZoom' is private
    Double_t       fZoom;                //  current zoom
                   ^
/tmp/cmsbuild/workspace/ib-any-integration/CMSSW_7_6_X_2015-09-27-2300/src/Fireworks/Core/src/FWEveView.cc:418:16: error: within this context
    s<<(camera->fZoom);
                ^


you can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-11520/8431/summary.html

[davidlt]

@VinInn I believe, some of the "target" classes are not from CMSSW code. E.g. Fireworks seems to look into ROOT classes internals.

There are other techniques (found after some googling):
(a) Make thing protected and then for testing use a wrapper class which makes things public, then you can access internals.
(b) Use friend keyword, i.e. make a test class to be a friend of the class you about to test. This cost you a single line in production code.
(c) There seems to be a template solution to access private/protected things used in Clang

Or even better, design classes in a way that they could be tested.

[davidlt]

Should we disallow people testing/accessing internals of various classes (CMSSW or not) or we should this to some degree? If allowed (e.g. for white-box testing), I could prepare some slides for showing possible alternatives for #define private public.

Personal opinion would be not to allow and classes should be tested by a public contact (API). If there is a need to test a lots of private/protected stuff, most likely such class needs refactoring. Now for non-test code, we should into refactoring it or/and creating such public API that there wouldn't be a need for #define private public.

@davidlange6 your opinion?

[namapane]

I have two of these in test code, the purpose was obviously to allow
test code to access internals that nobody else should be allowed to
access. The right solution would be to declare the test class a friend I
suppose, or is there a reason why this would not be OK?

N.

On 29-Sep-15 15:23, davidlt wrote:

Should we disallow people testing/accessing internals of various classes
(CMSSW or not) or we should this to some degree? If allowed (e.g. for
white-box testing), I could prepare some slides for showing possible
alternatives for |#define private public|.

Personal opinion would be not to allow and classes should be tested by a
public contact (API). If there is a need to test a lots of
private/protected stuff, most likely such class needs refactoring. Now
for non-test code, we should into refactoring it or/and creating such
public API that there wouldn't be a need for |#define private public|.

@davidlange6 https://github.com/davidlange6 your opinion?

—
Reply to this email directly or view it on GitHub
#11520 (comment).

[davidlt]

@namapane that's one way to go, e.g. such thing is allowed in Google C++ Test Framework [1].

Also the following should work, by having members as protected:

  class A {
  public:
    //
  protected:
    int i;
  };

The in test implementation:

  #include "A.hpp"

  class A_test : A {
  public:
    using i;
  };

You make it visible in public.

In once case it cost you to have a test a friend to the class you want to test, in other case you have to convert private to protected and then have a wrapper class in test code which would expose it as public.

Or you can make public APIs and just use those for testing :)

The last option templates to make things public: http://bloglitb.blogspot.ch/2010/07/access-to-private-members-thats-easy.html

[1] https://code.google.com/p/googletest/wiki/AdvancedGuide#Testing_Private_Code

[Dr15Jones]

I am strongly against making them protected. ROOT did that and it was extremely difficult to avoid problems.

[davidlt]

@Dr15Jones good input, but you elaborate on the issues faced on such method (for the record)? Did that gave too much freedom and people started to abusive it, i.e. changing the internal members of some ROOT classes?

[Dr15Jones]

Allowing protected access makes it very easy for inheriting classes to break class invariants. This was happening all the time in ROOT. When looking for threading problems, one didn't just need to look into the one class iself, but into all classes that directly or indirectly inherited from the class. This was extremely hard to maintain.

[namapane]

I personally would opt for friendship, in fact this specifes clearly
that I want to allow a test code to be able to fiddle with internals,
and nobody else. There's no reason why the test code should inherit and
that would also be problematic in some cases (e.g. when the parent has
data members that need to be initialized).

My question was if friendship is anything out of fashion nowadays for
whatever technical reason (which sounds like an interesting question in
a more general sense, if you want...)

N.

On 29-Sep-15 16:30, Chris Jones wrote:

rotected access makes it very easy for inheriting classes to break class
invariants. This was happening all the time in ROOT. When looking for
threading problems, one didn't just need to look into the one class
iself, but into all classes that directly or indirectly inherited from
the class. This was extremely hard to maintain.

[civanch]

I would simply add const access methods where possible to get necessary information for testing.

[wddgit]

Personally, I would prefer to allow friendship to be declared for these white-box unit tests. I also find these kinds of tests to be very useful in some cases.

[davidlt]

I believe, people can go ahead and start cleaning up the test code by using adding a test class or/and functions as friends to the class to be tested. Fireworks code will be handled separately (half of the stuff is already available via public APIs on ROOT side).

I would like to ask to add a reference from cleanup PRs to this one, that we could track the progress. Or this should be converted to an issue with a list of files to be cleaned up and all the references to PRs should go into that issue.

[davidlt]

To kickstart this I made 3 PRs cleaning 3 tests ( #11590 #11592 #11591 ) using functions/class friendship.

Solves issues on:

• DataFormats/TrackReco/test/testHitPattern.cpp
• CondCore/DBCommon/test/testBlobStreaming.cpp
• CalibTracker/SiStripDCS/test/UnitTests/TestSiStripDetVOffBuilder.cc

[Dr15Jones]

Here are all the framework ones
#11610

[davidlange6]

@davidlt - should we close this PR?

[davidlt]

Latest updates are:
#11641
#11659
root-project/root#99

Thus we slowly moving forward. Some categories have not yet cleaned up their files, I think.

[davidlt]

It's going to slow. I mane several PRs cleaning things:
#11950
#11951
#11952
#11953
#11954
#11955
#11956

[davidlt]

#11957

[davidlt]

I think, all or most of the fixes exist. Closing.