[zeromq-dev] ZeroMQ contribution policy - style guide

[john skaller]

On 05/02/2012, at 7:55 PM, asif saeed wrote:

> Hi All,
> 
> On Sun, Feb 5, 2012 at 2:12 AM, Martin Sustrik <sustrik at 250bpm.com> wrote:
> On 04/02/12 02:30, john skaller wrote:
> > I have just added a new section to the style guide "banning" the calling
> > of public functions by public functions. Public functions are reserved
> > for the public.
> Good idea. +1
> 
> I don't think that's a good idea. Public functions comprise a public interface that is supposed to stay the same. So what is the point in banning their use in implementation code? From C++ perspective, you can simply move  the functions implementing the functionality to the private section of your class - that way, they will remain hidden. Making something public and not allowing their use in implementation code suggests of code smells.

You are correct, you should move the function into the private section
if you want to use it in the implementation. If you also want it to be
public then you should provide a wrapper:

struct X { 
  int fact(int x) { return fact_impl(x); }
private:
  int fact_impl(x) { 
    if (x>0)  
      return x * fact_impl (x-1); 
     else 
      return 1; 
}

In this example fact_impl is recursive so it requires itself and 
therefore cannot be public. A wrapper is required. There is
good reason too: we decide to make it safer:

struct X { 
  int fact(int x) { 
   if (x>=0) 
     return fact_impl(x);
   else throw "Negative argument to factorial function"; 
  }
private:
  int fact_impl(x) { 
    if (x>0)  
      return x * fact_impl (x-1); 
     else 
      return 1; 
}

and you surely don't want the invariant check done every
iteration of the recursion.

With this structure you can PROVE by examining fact_impl,
that the invariant x >=0 is maintained. Therefore the check
isn't required.

There are a number of rules for proper use in C++, some of them
are actually broken by C++ itself. Another rule which is sure
to be controversial, because everyone gets it wrong including
esteemed textbook writers, is that virtual functions should always
be private.

C++ gets this wrong. The overrides to a virtual should be hidden,
not private: in other words it should be impossible to call an
virtual or any override of it from any derived class.

virtual functions are not just implementation details,
they're actually DATA (they're not even functions).
That is, calling a virtual function causes it to return a value
which is a part of the representation, and one which,
in any top level abstract class, has an unknown value.

Similarly any behaviour is a representation detail.
You know this if you do a class like:

	struct Drawable { 
   		void Draw() { draw(); }
	private:
		virtual void draw()=0; 
	}
	struct Circle: Drawable {
		Circle(int x_, int y_, int r_);
	private: 
		int x; int y; int r;
		void draw(); 
	}

It's pretty obvious now, since x,y,r are private coordinates and
circle radius, the function that draws the circle should be too.
To draw whatever is Drawable you should call Draw().

I didn't add that rule to the style guide because many people would
disagree. Also, in C++ if you do it right, you end up with quite a lot
of wrappers which act as level translations.

There are also rules about "protected" functions.

I have a class which is done *strictly* correctly.
It is a bit of a pain because of the level translations,
but this particular class is the most important in my whole
project, and it is worth the effort to get it right: it is the
system garbage collector, it is polymorphic, and it needs
to be utterly robust and at the same time very flexible.

A particular implementation is not thread safe, and a derived
class which wraps it with mutii (plural of mutex?) is.

The required structure is this:

struct A { 
  void f() { v_f(): } 
private:
  virtual void v_f()=0;
};

struct U : virtual A {
protected:
  void impl_f();
private:
  void v_f() { impl_f(); }
};
 
struct TS : U {
private:
  void v_f() { RAII_locker; impl_f(); }
}

If you examine this code carefully you'll see why it is done exactly this way.

This is how ALL virtual functions should be handled (except destructors).
It is the only way to correctly manage the invariants.

The excessive amount of wrapping is essential for the Open/Closed principle
to operate correctly: to have the abstraction and be able to implement it,
and also extend implementations.

So basically .. if you think "not calling public functions from public functions" is a smelly idea, 
you're going to think the above structure is totally stinko! 

The thing is, it is clearly not only as close to correct as possible in C++, 
it is also clearly the ONLY way to do it.

Of course, you'd never do this when prototyping. At that stage you'd be using
a plain old struct with public data members. You only provide all that mechanism
when the interface is totally locked down and you have use cases for the polymorphism.

It's common, and reasonable practice not to "go the whole hog" as I have illustrated
is actually correct, because it does make refactoring hard (it also makes modification
without refactoring much easier:) Therefore, the structure should be a guide: every C++
programmer should know it, and know when they're not doing it right, and why it is OK in their
circumstances to break the correct structure. Laziness is an argument. I'm lazy. I only use
the above structure for one type in my whole project :)


--
john skaller
skaller at users.sourceforge.net