[notmuch] [PATCH 2/4] notmuch: Config option to specify tags to be applied by 'notmuch new'.

[Carl Worth]

On Wed, 25 Nov 2009 22:50:08 +0100, Jan Janak  wrote:
> On 25-11 15:56, Bart Trojanowski wrote:
> > I really want this feature to get in, so I am going to do my best to
> > review your code :)
> > 
> > Here are some more sticking points...
> > 
> > > +char **
> > > +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length);
> > 
> > If you are not giving over control of the pointer to the caller please
> > return const char * const *.
> 
> I followed Carl's style there, in particular the following function:
> notmuch_config_get_user_other_email
>
> I can, of course, change that. But maybe we should wait for Carl to see
> which way he prefers.

Call me stupid. Anything more complex than "const char *" and I can
never figure out where to put the const to express what's desired.

If "const char * const *" means what we want it to, then feel free to
return that, (and fix up existing cases). But maybe add a little block
of example code to the documentation of the function so that idiots like
me can still figure out how to declare the right kind of variable to
call the function.

-Carl
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: 

[notmuch] [PATCH 2/4] notmuch: Config option to specify tags to be applied by 'notmuch new'.

[Jan Janak]

Hi Bart,

On 25-11 15:56, Bart Trojanowski wrote:
> Jan,
> 
> I really want this feature to get in, so I am going to do my best to
> review your code :)
> 
> Here are some more sticking points...
> 
> > +char **
> > +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length);
> 
> If you are not giving over control of the pointer to the caller please
> return const char * const *.

I followed Carl's style there, in particular the following function:
notmuch_config_get_user_other_email

I can, of course, change that. But maybe we should wait for Carl to see
which way he prefers.

> Similarly...
> 
> > +char **new_tags;
> 
> ... this should probably be const char **.

That's the same story. I followed user_other_email there.

> Next...
> 
> > +char **
> > +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length)
> 
> ... but ...
> 
> > +unsigned int count, i;
> > +
> > +if ((tags = notmuch_config_get_new_tags (config, )) == NULL)
> > +   return;
> 
> size_t != unsigned int on all platforms.  Please stick with one or the
> other.  Note there are a few calls to fix here.

That's a good catch. I will fix that one. Thanks a lot for the review, I
really appreciate that!

  -- Jan

[notmuch] [PATCH 2/4] notmuch: Config option to specify tags to be applied by 'notmuch new'.

[Bart Trojanowski]

Jan,

I really want this feature to get in, so I am going to do my best to
review your code :)

Here are some more sticking points...

> +char **
> +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length);

If you are not giving over control of the pointer to the caller please
return const char * const *.

Similarly...

> +char **new_tags;

... this should probably be const char **.

Next...

> +char **
> +notmuch_config_get_new_tags (notmuch_config_t *config, size_t *length)

... but ...

> +unsigned int count, i;
> +
> +if ((tags = notmuch_config_get_new_tags (config, )) == NULL)
> + return;

size_t != unsigned int on all platforms.  Please stick with one or the
other.  Note there are a few calls to fix here.

-Bart

-- 
WebSig: http://www.jukie.net/~bart/sig/