Code quality news

[Ken Raeburn]

On Jun 4, 2009, at 13:35, ghudson at MIT.EDU wrote:
> With the krb5 1.7 release out, we have the opportunity to spend a
> little time on general code quality before getting back into the press
> of new features and new release-critical bugs.  Here is some stuff
> we've been thinking about in meetings and/or working on:

Overall this looks very good!

> 2. Splitting up big source files

I think this is good to a point.  I don't really subscribe to the "one  
function per file" methodology that was so important in the days of  
static-only linking, expensive disks, and every admin having to  
compile and install Kerberos himself if he wanted the three or four  
existing Kerberos applications; now shared libraries are the norm, the  
linker can't selectively discard functions not in use by one  
particular application, and unless the functionalities are really  
distinct it's often helpful to see multiple functions together instead  
of in separate files.  As long as you're not taking it to an extreme,  
yes, splitting up source files with not-terribly-closely-related  
functionality is probably a good thing.  If it is closely related, I'd  
keep them together.

I also think it can be reasonable to use preprocessor directives for  
some subsetting.  Not in all cases, but maybe if you want a subset of  
very closely related functions.  (E.g., for cases like ccaches or  
keytabs where we use function-pointer tables, I'd probably keep the  
individual implementations collected together in one file each, and  
use #ifdef to eliminate certain operations not needed in a certain  
subset.  That is, unless you want to factor out some common code or  
algorithms used in multiple of these implementations as separate  
helper functions.)

I think I'd like to see k5-int.h broken up a lot.  Having one or two  
headers where you can get all the widely-used internal stuff is  
probably good, but it can include sub-headers with different types of  
functionality, and it can omit things that really could be localized  
to one directory or so.  The installed krb5.h, eh, maybe... it might  
encourage some developers to include only <krb5/this.h> and <krb5/ 
that.h> unless you take steps to make doing so difficult, and then you  
can never get rid of those headers if you want to reorganize again.   
The glibc folks have done it well, I think -- if you try to include  
the wrong header directly, certain cpp symbols don't get defined and  
it yells at you and your code doesn't compile.

> Some of the specialized applications of krb5 involve subsets of the
> libraries--a well-known example is the kernel krb5 code necessary to
> support the NFS client and server.  Although it's hard to structure
> the source code to handle every possible subset, it's generally easier
> to build subsets out of smaller pieces.

True, though a "piece" could be a function instead of a file.

>  * Helper functions used by several different API functions should
>    not live in the same source file as one of the API functions.

If the API functions have similar functionality, I think it makes  
sense to put them (and the support functions) all together in one  
file.  Unless you're going for the smallest-static-link goal.   
Subsetting by selecting certain files makes this trickier when you  
only want some of these API functions; but I think all of the  
solutions are kind of ugly in that case.  (I'm picturing something  
like get_creds here -- big "helper" functions with all the  
functionality, maybe broken into manageable pieces, wrapped with  
little API functions presenting nicer interfaces for the common use  
cases.  If the helper functions are common to various unrelated API  
functions, or the API functions involve a lot of non-common code,  
yeah, make them separate instead of treating them like they're closely  
tied to one use and not another.)

> 3. Smaller functions

I don't mind big functions for one big integrated piece of  
functionality with non-trivial logic, but that's usually not what  
we've got.  Breaking the bigger functions up into logically separate  
helpers would improve things a lot!

> 4. Function documentation
>
> We are thinking about adding doxygen markup before every function in
> our libraries, starting with placeholders.  This is a departure from a
> previous plan to add doxygen markup to our header files.  Non-API
> functions would be marked as such using the @internal special command.

I'm a little concerned about this.  Does doxygen let you specify "this  
part of the documentation for this function is internal"?  I could  
imagine some of the internal documentation including notes about  
details of the current implementation, and wanting to make such notes  
about functions that are exported, but not wanting to make them part  
of the API contract that application programmers will expect us to  
adhere to going forward.  Such notes shouldn't show up in the official  
API documentation.

-- 
Ken Raeburn / raeburn at mit.edu / no longer at MIT Kerberos Consortium