Re: Private library symbols...

[Sheldon Gill]

Richard Frith-Macdonald wrote:
> On 16 Oct 2006, at 04:13, Sheldon Gill wrote:
>
> > Hi Richard,
> >
> > You've made some recent changes to base with the idea of making 
> > private functions in base more obviously private and less accessible.
>
> Yes ... as obviously private as possible.
>
> > It seems to be that there is really only one goal with these changes:
> >
> >   applications/tools shouldn't be using private functions in base
> >
> > Is there any other objective that I'm missing?
>
> Well, that's the main tactical objective ... the strategic objective is 
> to improve maintainability ... so centralisation and clarification 
> internally is almost as much of a goal as simply trying to ensure that 
> external apps/tools don't use private stuff.

Okay.

> > If this is the case, I think the approach being taken here isn't the 
> > one which we should be pursuing.
> >
> > The original situation is we have functions like these:
> >
> > NSString  *GSLastError(int);
> > BOOL       GSEnvironmentFlag( const char *, BOOL );
> >
> > which make these functions seem like GNUstep API additions but really 
> > they are private.
> >
> > What has been done is create an "artificial" object
> >
> > @interface _GSPrivate : NSObject
> > + (NSString*) error;
> > @end
> >
> > @interface _GSPrivate (ProcessInfo)
> > + (BOOL) environmentFlag: (const char *)name defaultValue: (BOOL)def;
> > @end
>
> Yes, the idea is to collect everything together as much as possible, 
> making it easier to find these things and making it clear that the 
> various mechanisms are used internally.
>
> > Now I say artificial because it doesn't conform to an object design. 
> > There never is an actual instantiation, for example, so you're always 
> > sending messages to the class. There are many other things which make 
> > this not really an object.
>
> This is a work in progress ... I'm not sure yet whether it would be good 
> to have an instance and use instance methods rather than just a class 
> and use class methods.  The main reason for using a class is to use 
> language features to support encapsulation and maintainability rather 
> than depending solely on conventions.

Well, you  actually are relying on conventions anyway. You've called the class 
_GSPrivate using the underscore convention to indicate privacy.

Also, these are private function calls within the library. Encapsulation 
doesn't really apply here.

> > Doing this adds many more bytes to the library.
> > It also makes these method lookups rather than function calls so they 
> > are slower.
>
> True, but these are very minor overheads and I think clarity and 
> maintainability come first before looking at optimisation (obviously we 
> can optimise by caching method IMPs or by using a struct as a dispatch 
> table rather than using a class).

I'm quite aware of how we can optimise method calls but this isn't really 
optimisation.

I also find this inconsistent with your previous statements about avoiding 
'bloat', as you put it. Surely more size and less speed for no functional 
changes is the very essence of 'bloat'?

I don't agree that these changes make anything more maintainable or easier than 
simply decorating the function calls with an underscore in the conventional way.

> > So we've added an artificial/strange object, increased the library 
> > size and slowed down those function calls all in an effort to prevent 
> > applications using our private functions. Is this right?
>
> As you can tell from my answers above, it's only partially right.
>
> > Now its long been idomatic to use the underscore to mark private 
> > symbols. In fact, this is precisely what you've done with the class name.
>
> Yes, I've tried to use every signal I could think of  ... the leading 
> underscore, the word 'Private' the exclusion from any external headers 
> and reduced linker symbols, and the inclusion of comments.
>
> > Further, I think this makes navigating the source harder. Quick, tell 
> > me without searching where you're going to find the implementation for 
> > the above two functions?
>
> I'm very surprised at this comment ... since a big part of the intent is 
> to make navigating the source *easier* and I believe this is achieved.
> I certainly think it's not obvious where the old functions were ... but 
> the new methods are clear since the category names tell you where the 
> individual method implementations in each group are, the declarations 
> are all in a single place, and you can find all the private methods 
> implementations easily using a search for @implementation _GSPrivate.  

Making sure all declarations are in GSPrivate.h is good but a matter of 
programmer discipline and has no bearing on the function call vs class method 
issue.

> That's a big improvement over the older code.

Conventional layout has private functions near the top of the source file, just 
after header includes. They aren't that hard to find. Especially if you keep to 
convention with the underscores.

Sure, the category names help you find where the particular function is 
located. I've an equivalent way which adds precisely 0 bytes to the libnrary 
and makes no impact on the runtime:

// from NSProcessInfo
BOOL _environment_flag(const char *, BOOL );


> > Why don't we simply change the functions to conform to idomatic norms?
> >
> > NSString  *_GSLastError(int);
> > BOOL       _GSEnvironmentFlag( const char *, BOOL );
> >
> > and be done with it?
>
> Well, it fails to centralise and make it easier to find things.  It''s 
> not as clear (in fact meaningless to people not familiar with the 
> underscore), and it leaves these as global symbols (easier for people to 
> use externally).

How does it fail to centralise? The implementations can be where ever they need 
to be, including in a single 'centralised' file. You can 'centralise' the 
declarations in GSPrivate.h

As to the leading underscore, that is used by C and Objective-C and extensively 
within -core. Who is going to be working with the code who doesn't know the 
language? Are you advocating that we change the way -core is written so that 
its easy for those not familiar with the language?  That we should eschew idiom 
and use different approaches to facilitate that?

And what is the real problem with the global symbols? Are you suggesting that 
people look through the library symbol table and choose methods/functions based 
on what they find there? And that it is an issue for -core maintainers to address?

> > This won't add to size.
> > This won't slow things down.
> > This stays in convention.
> >
> > If you really want, we could further decorate along the lines of:
> >
> > NSString  *_private_GSLastError(int);
> > BOOL       _private_GSEnvironmentFlag( const char *, BOOL );
> >
> > although I don't see the point of doing that. I do see some merit in 
> > making these private functions GNU convention
> >
> > NSString  *_gs_last_error(int);
> > BOOL       _gs_environment_flag( const char *, BOOL );
> >
> > as that is keeping with the style guide.
>
> I think the GNUstep style guide allows you to do what you .like with 
> private functions, but consistency with the rules for public ones would 
> suggest that the embedded underscores in the above examples ought to be 
> replaced with capitalised letters ... _privateGSLastError, 
> _privateGSEnvironmentFlag, _GSLastError, and _GSEnvironmentFlag.
> I guess it's debatable whether we are talking about private or public 
> functions here ... they are certainly intended to be private, but as 
> long as they are available for external applications to link to, they 
> are in at least some sense public.

All methods in objective-c are in at least some sense public.

> > Further, if the problem is, as you say, "symbol pollution" why not 
> > stop the pollution where it really is occurring: the exports symbol 
> > table? Its a linker problem, not really a source code problem.
>
> To be honest, that simply didn't occur to me, as I don't know how to do 
> it (or even that it could be reasonably/portably done).

Oh?

> What would be involved in this?  If we *can* remove symbols effectively 
> without breaking the library's internal use of them or impairing 
> debugging, it would probably be good to do so.

Try:

$> strip

Just to be clear, I'm all for improving the code for reading, navigation and 
maintenance. I simply disagree that this class wrapping is the right way to do 
it. We can achieve all those goals without the class wrapper.


Regards,
Sheldon