static and HIDDEN

[Greg Banks]

On Thu, Jul 12, 2012, at 03:05 PM, Дилян Палаузов wrote:
> Hello,
> 
> > Dilyan, this warning is caused by some of your recent commits, like
> >
> > libcyrus: mark all local symbols declared in .h with static
> >
> > which made some of the function declarations in header files 'static'.
> > That's just never right.
> >
> > Also, you've marked a number of cyrusdb_*() functions as 'static'.
> > These are actual external APIs used by code, albeit in the conversations
> > branch, not master.  This is just not helpful.  If you care that much
> > about external symbols, remove the functions entirely and we'll put them
> > back when we merge the conversations code.  Or just leave them alone.
> 
> Yes, attaching static to all variables and functions, that are not used 
> out of the library, was sometimes inappropriate.  I do not have problems 
> with either of the approaches to remove the functions entirely, or to 
> mark them as part of the external API, which will be used by future 
> versions.  On the other side, there shall be some track of functions, 
> which are really unnecessary (like lib/util.c:kv_bsearch()), and marking 
> them with HIDDEN or static eases their finding.  

That particular one doesn't seem to have ever been used.  It was
introduced by

commit 307c689e3e099f437a732bcc659be8f90549a5f2
Author: John Gardiner Myers <jm36 at andrew.cmu.edu>
Date:   Mon Mar 14 22:47:51 1994 +0000

    Initial revision

and was not used then, nor has any other code used it since.  It appears
to have copied in from some other code at CMU.


> I do not know which of the remaining HIDDEN symbols (in libcyrus and 
> libcyrus_imap; in cyrus_min and cyrus_sieve internal symbols are not 
> annotated), belong to future external API, and which will be strictly 
> internal for the library but are included in a header file

Agreed, it's very hard to tell. It doesn't help that the FastMail
practice is to put changes that affect common code, e.g. by adding new
functions, into separate commits, which after a few days or weeks end up
in the master branch without the code that needs them.  Nor does it help
that in this particular case the new functions were added but no unit
tests were added for them (in any branch).  Nor does it help that lots
of development is occurring off the master branch, and not even on
git.cyrusimap.org.

Bron's latest stuff is usually at

https://github.com/brong/cyrus-imapd/tree/fastmail

and here's the branch I'm working on

https://github.com/gnb/cyrus-imapd/commits/search

Other than that, there's not much help I can offer -- Cyrus is just old
and dusty and untidy and you have to dig around to figure anything out
and every now and again you discover a landmine.

> (e.g. why is 
> fuzzy_match() part of lmtpd.h, when it is defined and used entirely in 
> lmtpd.c and thus shall be static?).

That's probably historical -- a lot of Cyrus code was written decades
ago when compilers had fewer warnings.  In K&R C.  On CVS.  Probably
before CVS too.

Using git-blame -s I see the declaration in the header was added in

commit 51791d1e012695e15ff375f42dcfe121630f1a4d
Author: Ken Murchison <murch at andrew.cmu.edu>
Date:   Thu Oct 20 15:29:01 2005 +0000

    Added support for Sieve scripts on shared mailboxes via the
    /vendor/cmu/cyrus-imapd/sieve annotation

but the definition was added in

commit eed0f295b090a3cbee5e2298001c7692fb62435f
Author: Ken Murchison <murch at andrew.cmu.edu>
Date:   Thu Nov 30 17:11:16 2006 +0000

    moved 2.3 code to trunk

which was before the CVS -> git migration so the CVS branch merge
basically lost all useful development history.  But it looks like the
function was never used outside lmtpd.c, so probably it should always
have been static and never should have been added to the header.

I think your best bet is to use git-blame -s and git-show to work out
when the function was first added (as opposed to moved from another
file, or had it's declaration changed to ANSI C or some such change). 
If it was added in the last three years, your first guess should be that
it was made extern deliberately, otherwise that it was an accident.

-- 
Greg.