Date: Wed, 23 Oct 2013 15:32:09 -0400
From: Austin Clements <amdragon@MIT.EDU>
To: Tomi Ollila <tomi.ollila@iki.fi>
Subject: Re: [PATCH 1/3] cli: add insert --must-index option
Message-ID: <20131023193209.GF20337@mit.edu>
References: <1374365254-13227-1-git-send-email-novalazy@gmail.com>
	<87ip048gbj.fsf@qmul.ac.uk>
	<20130727151510.GA13750@hili.localdomain>
	<87hadtxfrr.fsf@qmul.ac.uk>
	<20130912001349.GA18821@hili.localdomain>
	<87zjqhv264.fsf@zancas.localnet>
	<m238o9fguj.fsf@guru.guru-group.fi> <87bo2xtdp2.fsf@unb.ca>
	<m2eh7bu7t5.fsf@guru.guru-group.fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <m2eh7bu7t5.fsf@guru.guru-group.fi>
User-Agent: Mutt/1.5.21 (2010-09-15)
Cc: notmuch@notmuchmail.org
Precedence: list

Quoth Tomi Ollila on Oct 23 at 10:05 pm:
> On Thu, Oct 10 2013, David Bremner <david@tethera.net> wrote:
> 
> > Tomi Ollila <tomi.ollila@iki.fi> writes:
> >>> I'm not opposed to doing an SONAME bump for 0.17. Are there other ABI
> >>> breaking changes that we have been holding back on? Can these maybe go
> >>> through at the same time?
> >>
> >> Maybe something along these lines...
> >>
> >> (Quick draft for the API part; to start discussion before working too much
> >> for something that is going to be dropped...)
> >>
> >>  notmuch_status_t
> >> -notmuch_database_create (const char *path, notmuch_database_t **database);
> >> +notmuch_database_create (const char *path,
> >> +			 notmuch_loghook_t *loghook,
> >> +			 notmuch_database_t **database);
> >
> > Another idea floated (by Austin?) was to pass in an options struct, to
> > allow future options to be added without changing the function
> > signature. I guess with some care this could be done in an upwardly
> > compatible way.
> 
> Maybe something like
> 
> #define NOTMUCH_API_OPTIONS_VERSION 1
> typedef struct {
>         int options_version;
>         void (*log)(void *, int level, int status, const char * format, ...);
>         void * logdata;
> } notmuch_options_t;        
> 
> ...
> 
> notmuch_status_t
> notmuch_database_create (const char *path,
> 			 notmuch_options_t *options,
> 			 notmuch_database_t **database);
> ...
> 
> notmuch_status_t
> notmuch_database_open (const char *path,
>                        notmuch_database_mode_t mode,
>                        notmuch_options_t *options,
>                        notmuch_database_t **database);
> 
> then in use:
> 
> notmuch_options_t options = {
>        .options_version = NOTMUCH_API_OPTIONS_VERSION,
> 
> .. äsh, this has problem that the macro changes in header file
> but the structure initialization is not following automatically
> (in other fields than that). Therefore perhaps "fixing"
> the version macros:
> 
> #define NOTMUCH_API_OPTIONS_VERSION_1 1
> /* #define NOTMUCH_API_OPTIONS_VERSION_2 2 // added in the future */
> 
> notmuch_options_t options = {
>        .options_version = NOTMUCH_API_OPTIONS_VERSION_1,
>        .log = log_to_stderr,
>        .logdata = NULL
> };
> 
> Well, this is one idea (does not sound as good as I initially
> thought...) how does other software tackle this kind of issues...
> 
> If something like this is finally chosen we could provide easy
> transition path to allow NULL as options -- making notmuch CLI
> behave as it used to be (log to stderr...).

Using a version number on the options makes it tricky to maintain
backwards compatibility, since you need code to read all past versions
of the structure.  A similar but, I think, better way would be to take
the size of the structure in the structure.  Something like:

typedef struct {
    size_t options_length;
    ...
} notmuch_options_t;

#define NOTMUCH_OPTIONS_INIT { sizeof(notmuch_options_t) }
// or without pre-processor, but requiring GCC attributes
static __attribute__((__unused__))
notmuch_options_t notmuch_options_init = { sizeof(notmuch_options_t) };

NOTMUCH_OPTIONS_INIT/notmuch_options_init could also contain other
defaults, though it's best if the defaults are simply the zero
initializations of the various fields.


Then, in code calling libnotmuch, you'd do something like

notmuch_options_t options = NOTMUCH_OPTIONS_INIT;
options.log = log_to_stderr;
// ...


And in libnotmuch, we would do something like

notmuch_status_t
notmuch_database_open (const char *path,
                       notmuch_database_mode_t mode,
                       const notmuch_options_t *options,
                       notmuch_database_t **database)
{
    notmuch_option_t real_options = NOTMUCH_OPTIONS_INIT;
    if (real_options.options_length < options.options_length)
        return error;
    memmove(&real_options, options, options.options_length);
    // ...
}

This approach makes it free to add fields and we can always deprecate
fields as long as we leave the space in the structure.  This is based
roughly on how the Linux kernel deals with various structures that
have grown over time in the syscall ABI.

Another much more verbose but also more robust approach would be to
make notmuch_options_t opaque and provide setters (and getters, if
we're feeling benevolent).  I kind of like the simplicity of a struct.

One thing to consider is what works best with bindings.  Presumably
the opaque structure wouldn't be a problem.  I've never had to wrap
anything like the struct approach, though, so I don't know if that's
easy or hard.