[PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

On 05/18/2012 04:20 AM, Jani Nikula wrote:
> We have -Wextra, which enables -Wmissing-field-initializers, which
> requires us to use full initialization of struct fields when doing
> regular, non-designated initialization. The point is that you might
> introduce subtle bugs if you added new struct fields and forgot to check
> the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
> of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

i think we can agree that this is the right choice.  We might even want
to discourage non-designated initializations entirely.

> IMHO the whole point of designated initializers is that the
> initialization is not vulnerable to struct changes, and you can pick
> which fields you choose to initialize explicitly. Also, it has the added
> benefit of documenting the fields that are initialized, without having
> to look at the struct definition.

Agreed.

> Do we now want to initialize all struct fields explicitly, everywhere,
> even when using designated initializers? Isn't that the question then?

I'm not sure it has to be this dramatic and "all or nothing".  For
example, it could be reasonable to explicitly initialize some subobjects
and not others.  For example, the notmuch_crypto_t jamie is proposing
would effectively encode the default setting for the --verify and
--decrypt flags.  I could see wanting to explicitly initialize those
default policy choices, even if they happen to be identical to the
implicit "zero"ing.

> Won't that maintain and promote the misconception that explicit
> initialization is required, when it's really not, failing to educate the
> non-experts and planting a seed of doubt in the experts...?

i see your point here, which is why i'm not arguing that all subobjects
need to be explicitly initialized all the time.

> It's not always clear whether something is a matter of taste, style, or
> language paradigm. If it feels like a paradigm, sticking with it
> ultimately benefits *both* perspectives.

yep, understood.

--dkg

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1030 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Tomi Ollila]

On Fri, May 18 2012, Daniel Kahn Gillmor  wrote:

>
> The real tradeoff in this choice is whether we prefer:
>
>  a) more compact code to facilitate quick reading by experts
>
>or
>
>  b) more verbose code to facilitate comprehension by the non-expert.
>
> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P
>
> Overall, i think i'm still in the (b) camp.  But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.

I also think it is good to see explicit initializations when those aren't
needed but clarifies the code. After all it doesn't generate any extra
code to the target module. Also the &(params->crypto) is good from
clarification point of view.

Austin's .crypto { ... } initialization looks good & clear; In case there
will be new version of this patch series I'd like to see that used...

>   --dkg
>
> PS gcc's -pedantic argument provides the following warning:
>
>  error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.

In order to do that id:"cover.1325977940.git.jani at nikula.org" needs to
be applied (and probably rebased).

> [0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

Tomi

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

*sigh* I'm failing to detach myself from this conversation. :(

On Thu, 17 May 2012, Daniel Kahn Gillmor  wrote:
> I don't think it's an assumption -- Jani is probably relying on the C
> standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

Thanks for digging up the references.

> The real tradeoff in this choice is whether we prefer:
>
>  a) more compact code to facilitate quick reading by experts
>
>or
>
>  b) more verbose code to facilitate comprehension by the non-expert.

We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.

Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?

> I started this discussion leaning strongly toward the (b) perspective.
> But now that i know the relevant bits of the standard, i can sympathize
> with the (a) perspective as well :P

It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.

> Overall, i think i'm still in the (b) camp.  But i think it's more
> important that we don't allow dithering over this issue to prevent the
> inclusion of this patch series, which is a step in the right direction
> for handling S/MIME messages as well as PGP/MIME.

Agreed.

> PS gcc's -pedantic argument provides the following warning:
>
>  error: ISO C90 forbids specifying subobject to initialize
>
> So we probably want to specify -std=c99 at least to ensure our choice of
> subobject initialization is respected.

Unfortunately, the notmuch code base uses mixed standards, due to GCC
being so lax about it. Anything -pedantic produces warnings:
id:"cover.1325977940.git.jani at nikula.org". You may also want to try
clang to get better warnings.


BR,
Jani.

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins  
wrote:
> On Thu, May 17 2012, Jani Nikula  wrote:
>> The values are not undefined, they are properly initialized, and we can
>> count on it. For sure, not maybe. If you want to explicitly set them for
>> clarity, it's a matter of taste. Personally I find it too verbose, but then
>> again notmuch code is generally fairly verbose.
>
> I want them explicitly set for clarity, as well as safety.  Code is
> meant to be read by humans, not computers.  Brevity is not always a
> virtue if it sacrifices clarity.  It's much nicer to have the defaults
> clearly stated in the initialization, than to force the reader to
> understand how the initialization works and to interpret what that means
> for the current case.  I also don't think it's safe to assume that the
> variables will be always be "properly" initialized in your favor in
> perpetuity.  It's much safer to explicitly set them to what you want
> them to be rather than just assume they'll be set correctly.

In short, when you read enough code, having everything explicitly stated
becomes a burden. It's explicit, but you have to read it all, even when
there really is no need to.

>> If you insist on it, please at least drop the extra temp crypto
>> variable, and initialize the struct in one initializer.
>
> I don't see why this matters either.  Again, I think this is just a
> matter of taste.  I would rather the code be verbose where clarity
> requires it, rather than always trying to make the code as terse as
> possible.

You introduce an extra variable that every reader of your code has to
track down to realize that it's only ever used once to initialize
another variable. You make code harder for other people to read.

I have now offered my review and opinions on the matter; I will not
pursue this discussion further.


BR,
Jani.

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Tomi Ollila]

On Fri, May 18 2012, Daniel Kahn Gillmor d...@fifthhorseman.net wrote:


 The real tradeoff in this choice is whether we prefer:

  a) more compact code to facilitate quick reading by experts

or

  b) more verbose code to facilitate comprehension by the non-expert.

 I started this discussion leaning strongly toward the (b) perspective.
 But now that i know the relevant bits of the standard, i can sympathize
 with the (a) perspective as well :P

 Overall, i think i'm still in the (b) camp.  But i think it's more
 important that we don't allow dithering over this issue to prevent the
 inclusion of this patch series, which is a step in the right direction
 for handling S/MIME messages as well as PGP/MIME.

I also think it is good to see explicit initializations when those aren't
needed but clarifies the code. After all it doesn't generate any extra
code to the target module. Also the (params-crypto) is good from
clarification point of view.

Austin's .crypto { ... } initialization looks good  clear; In case there
will be new version of this patch series I'd like to see that used...

   --dkg

 PS gcc's -pedantic argument provides the following warning:

  error: ISO C90 forbids specifying subobject to initialize

 So we probably want to specify -std=c99 at least to ensure our choice of
 subobject initialization is respected.

In order to do that id:cover.1325977940.git.j...@nikula.org needs to
be applied (and probably rebased).

 [0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

Tomi
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

*sigh* I'm failing to detach myself from this conversation. :(

On Thu, 17 May 2012, Daniel Kahn Gillmor d...@fifthhorseman.net wrote:
 I don't think it's an assumption -- Jani is probably relying on the C
 standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

Thanks for digging up the references.

 The real tradeoff in this choice is whether we prefer:

  a) more compact code to facilitate quick reading by experts

or

  b) more verbose code to facilitate comprehension by the non-expert.

We have -Wextra, which enables -Wmissing-field-initializers, which
requires us to use full initialization of struct fields when doing
regular, non-designated initialization. The point is that you might
introduce subtle bugs if you added new struct fields and forgot to check
the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

IMHO the whole point of designated initializers is that the
initialization is not vulnerable to struct changes, and you can pick
which fields you choose to initialize explicitly. Also, it has the added
benefit of documenting the fields that are initialized, without having
to look at the struct definition.

Do we now want to initialize all struct fields explicitly, everywhere,
even when using designated initializers? Isn't that the question then?
Won't that maintain and promote the misconception that explicit
initialization is required, when it's really not, failing to educate the
non-experts and planting a seed of doubt in the experts...?

 I started this discussion leaning strongly toward the (b) perspective.
 But now that i know the relevant bits of the standard, i can sympathize
 with the (a) perspective as well :P

It's not always clear whether something is a matter of taste, style, or
language paradigm. If it feels like a paradigm, sticking with it
ultimately benefits *both* perspectives.

 Overall, i think i'm still in the (b) camp.  But i think it's more
 important that we don't allow dithering over this issue to prevent the
 inclusion of this patch series, which is a step in the right direction
 for handling S/MIME messages as well as PGP/MIME.

Agreed.

 PS gcc's -pedantic argument provides the following warning:

  error: ISO C90 forbids specifying subobject to initialize

 So we probably want to specify -std=c99 at least to ensure our choice of
 subobject initialization is respected.

Unfortunately, the notmuch code base uses mixed standards, due to GCC
being so lax about it. Anything -pedantic produces warnings:
id:cover.1325977940.git.j...@nikula.org. You may also want to try
clang to get better warnings.


BR,
Jani.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

On 05/18/2012 04:20 AM, Jani Nikula wrote:
 We have -Wextra, which enables -Wmissing-field-initializers, which
 requires us to use full initialization of struct fields when doing
 regular, non-designated initialization. The point is that you might
 introduce subtle bugs if you added new struct fields and forgot to check
 the initializations. (This is why we have e.g. { 0, 0, 0, 0, 0 } instead
 of just { 0 } in the initialization of notmuch_opt_desc_t arrays.)

i think we can agree that this is the right choice.  We might even want
to discourage non-designated initializations entirely.

 IMHO the whole point of designated initializers is that the
 initialization is not vulnerable to struct changes, and you can pick
 which fields you choose to initialize explicitly. Also, it has the added
 benefit of documenting the fields that are initialized, without having
 to look at the struct definition.

Agreed.

 Do we now want to initialize all struct fields explicitly, everywhere,
 even when using designated initializers? Isn't that the question then?

I'm not sure it has to be this dramatic and all or nothing.  For
example, it could be reasonable to explicitly initialize some subobjects
and not others.  For example, the notmuch_crypto_t jamie is proposing
would effectively encode the default setting for the --verify and
--decrypt flags.  I could see wanting to explicitly initialize those
default policy choices, even if they happen to be identical to the
implicit zeroing.

 Won't that maintain and promote the misconception that explicit
 initialization is required, when it's really not, failing to educate the
 non-experts and planting a seed of doubt in the experts...?

i see your point here, which is why i'm not arguing that all subobjects
need to be explicitly initialized all the time.

 It's not always clear whether something is a matter of taste, style, or
 language paradigm. If it feels like a paradigm, sticking with it
 ultimately benefits *both* perspectives.

yep, understood.

--dkg



signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On May 17, 2012 5:26 PM, "Jameson Graef Rollins" 
wrote:
>
> On Thu, May 17 2012, Jani Nikula  wrote:
> > On Thu, 17 May 2012, Jameson Graef Rollins 
wrote:
> >> This makes sure it has proper initialization values when it's created.
> >
> > Please don't do this. It's unnecessary; if one field is initialized with
> > a designated initializer, the rest are initialized to zero (or NULL).
>
> It may be technically unnecessary, but why is that a reason to not do
> it?  I intentionally did this to make it clear what the defaults are.
> Otherwise the defaults are essentially undefined, which is not good.
> Maybe the structure initializes to the correct defaults, but why count
> on that when we can set them to the correct default, and have it clear
> to readers of the code?

The values are not undefined, they are properly initialized, and we can
count on it. For sure, not maybe. If you want to explicitly set them for
clarity, it's a matter of taste. Personally I find it too verbose, but then
again notmuch code is generally fairly verbose. If you insist on it, please
at least drop the extra temp crypto variable, and initialize the struct in
one initializer.

BR,
Jani.

>
> jamie.
-- next part --
An HTML attachment was scrubbed...
URL: 

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Austin Clements]

Quoth Jameson Graef Rollins on May 16 at  2:55 pm:
> This makes sure it has proper initialization values when it's created.
> ---
>  notmuch-reply.c |5 -
>  notmuch-show.c  |   10 +-
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6662adb..3c967a0 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  char *query_string;
>  int opt_index, ret = 0;
>  int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
> notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
> -notmuch_crypto_t crypto = { .decrypt = FALSE };
> +notmuch_crypto_t crypto = {
> + .decrypt = FALSE,
> + .gpgctx = NULL,
> +};
>  int format = FORMAT_DEFAULT;
>  int reply_all = TRUE;
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 8b4d308..c606333 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), 
> unused (char *argv[]))
>  char *query_string;
>  int opt_index, ret;
>  const notmuch_show_format_t *format = _text;
> -notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
> +notmuch_crypto_t crypto = {
> + .decrypt = FALSE,
> + .gpgctx = NULL,
> +};
> +notmuch_show_params_t params = {
> + .part = -1,
> + .omit_excluded = TRUE,
> + .crypto = crypto,
> +};

You can omit the temporary variable and avoid the struct copy by doing
something like this:

notmuch_show_params_t params = {
.part = -1,
.omit_excluded = TRUE,
.crypto = {
.decrypt = FALSE,
.gpgctx = NULL,
},
};

>  int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>  notmuch_bool_t verify = FALSE;
>  int exclude = EXCLUDE_TRUE;

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

On 05/17/2012 12:45 PM, Jameson Graef Rollins wrote:
> I want them explicitly set for clarity, as well as safety.  Code is
> meant to be read by humans, not computers.  

I sympathize with this sentiment.

> It's much safer to explicitly set them to what you want
> them to be rather than just assume they'll be set correctly.

I don't think it's an assumption -- Jani is probably relying on the C
standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

  all subobjects that are not initialized explicitly shall be
  initialized implicitly the same as objects that have static
  storage duration.

the latter clause references 6.7.8.10, which says:

   If an object that has static storage duration is not
   initialized explicitly, then:
 ? if it has pointer type, it is initialized to a null pointer;
 ? if it has arithmetic type, it is initialized to (positive or
   unsigned) zero;
 ? if it is an aggregate, every member is initialized
   (recursively) according to these rules;

So it's not just "an assumption", it's a guarantee from the underlying
language standard.

That said, it's a guarantee i was unaware of until i researched this.
I'm certainly not a C guru, but i've internalized a fair amount of C's
rules and structure and i'd never heard of this
subobject-default-initialization-when-other-subobjects-are-initialized
rule before.   If i'd seen the uninitialized members of the struct, and
that they were being used without explicit initialization, i would have
had to do a bit of digging to understand what's happening.

The real tradeoff in this choice is whether we prefer:

 a) more compact code to facilitate quick reading by experts

   or

 b) more verbose code to facilitate comprehension by the non-expert.

I started this discussion leaning strongly toward the (b) perspective.
But now that i know the relevant bits of the standard, i can sympathize
with the (a) perspective as well :P

Overall, i think i'm still in the (b) camp.  But i think it's more
important that we don't allow dithering over this issue to prevent the
inclusion of this patch series, which is a step in the right direction
for handling S/MIME messages as well as PGP/MIME.

--dkg

PS gcc's -pedantic argument provides the following warning:

 error: ISO C90 forbids specifying subobject to initialize

So we probably want to specify -std=c99 at least to ensure our choice of
subobject initialization is respected.

[0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 1030 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula  wrote:
> In short, when you read enough code, having everything explicitly
> stated becomes a burden. It's explicit, but you have to read it all,
> even when there really is no need to.

Not everyone who reads the code is an expert.  I think it's important
for the code to be clear to everyone.  Experts can more easily gloss
over the trivial parts than novices can divine things that are hidden.

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

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins  
wrote:
> This makes sure it has proper initialization values when it's created.

Please don't do this. It's unnecessary; if one field is initialized with
a designated initializer, the rest are initialized to zero (or NULL).

BR,
Jani.


> ---
>  notmuch-reply.c |5 -
>  notmuch-show.c  |   10 +-
>  2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/notmuch-reply.c b/notmuch-reply.c
> index 6662adb..3c967a0 100644
> --- a/notmuch-reply.c
> +++ b/notmuch-reply.c
> @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
>  char *query_string;
>  int opt_index, ret = 0;
>  int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
> notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
> -notmuch_crypto_t crypto = { .decrypt = FALSE };
> +notmuch_crypto_t crypto = {
> + .decrypt = FALSE,
> + .gpgctx = NULL,
> +};
>  int format = FORMAT_DEFAULT;
>  int reply_all = TRUE;
>  
> diff --git a/notmuch-show.c b/notmuch-show.c
> index 8b4d308..c606333 100644
> --- a/notmuch-show.c
> +++ b/notmuch-show.c
> @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), 
> unused (char *argv[]))
>  char *query_string;
>  int opt_index, ret;
>  const notmuch_show_format_t *format = _text;
> -notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
> +notmuch_crypto_t crypto = {
> + .decrypt = FALSE,
> + .gpgctx = NULL,
> +};
> +notmuch_show_params_t params = {
> + .part = -1,
> + .omit_excluded = TRUE,
> + .crypto = crypto,
> +};
>  int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
>  notmuch_bool_t verify = FALSE;
>  int exclude = EXCLUDE_TRUE;
> -- 
> 1.7.10
>
> ___
> notmuch mailing list
> notmuch at notmuchmail.org
> http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula  wrote:
> The values are not undefined, they are properly initialized, and we can
> count on it. For sure, not maybe. If you want to explicitly set them for
> clarity, it's a matter of taste. Personally I find it too verbose, but then
> again notmuch code is generally fairly verbose.

I want them explicitly set for clarity, as well as safety.  Code is
meant to be read by humans, not computers.  Brevity is not always a
virtue if it sacrifices clarity.  It's much nicer to have the defaults
clearly stated in the initialization, than to force the reader to
understand how the initialization works and to interpret what that means
for the current case.  I also don't think it's safe to assume that the
variables will be always be "properly" initialized in your favor in
perpetuity.  It's much safer to explicitly set them to what you want
them to be rather than just assume they'll be set correctly.

> If you insist on it, please at least drop the extra temp crypto
> variable, and initialize the struct in one initializer.

I don't see why this matters either.  Again, I think this is just a
matter of taste.  I would rather the code be verbose where clarity
requires it, rather than always trying to make the code as terse as
possible.

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

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula  wrote:
> On Thu, 17 May 2012, Jameson Graef Rollins  
> wrote:
>> This makes sure it has proper initialization values when it's created.
>
> Please don't do this. It's unnecessary; if one field is initialized with
> a designated initializer, the rest are initialized to zero (or NULL).

It may be technically unnecessary, but why is that a reason to not do
it?  I intentionally did this to make it clear what the defaults are.
Otherwise the defaults are essentially undefined, which is not good.
Maybe the structure initializes to the correct defaults, but why count
on that when we can set them to the correct default, and have it clear
to readers of the code?

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

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins jroll...@finestructure.net wrote:
 This makes sure it has proper initialization values when it's created.

Please don't do this. It's unnecessary; if one field is initialized with
a designated initializer, the rest are initialized to zero (or NULL).

BR,
Jani.


 ---
  notmuch-reply.c |5 -
  notmuch-show.c  |   10 +-
  2 files changed, 13 insertions(+), 2 deletions(-)

 diff --git a/notmuch-reply.c b/notmuch-reply.c
 index 6662adb..3c967a0 100644
 --- a/notmuch-reply.c
 +++ b/notmuch-reply.c
 @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
  char *query_string;
  int opt_index, ret = 0;
  int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
 notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
 -notmuch_crypto_t crypto = { .decrypt = FALSE };
 +notmuch_crypto_t crypto = {
 + .decrypt = FALSE,
 + .gpgctx = NULL,
 +};
  int format = FORMAT_DEFAULT;
  int reply_all = TRUE;
  
 diff --git a/notmuch-show.c b/notmuch-show.c
 index 8b4d308..c606333 100644
 --- a/notmuch-show.c
 +++ b/notmuch-show.c
 @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), 
 unused (char *argv[]))
  char *query_string;
  int opt_index, ret;
  const notmuch_show_format_t *format = format_text;
 -notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
 +notmuch_crypto_t crypto = {
 + .decrypt = FALSE,
 + .gpgctx = NULL,
 +};
 +notmuch_show_params_t params = {
 + .part = -1,
 + .omit_excluded = TRUE,
 + .crypto = crypto,
 +};
  int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
  notmuch_bool_t verify = FALSE;
  int exclude = EXCLUDE_TRUE;
 -- 
 1.7.10

 ___
 notmuch mailing list
 notmuch@notmuchmail.org
 http://notmuchmail.org/mailman/listinfo/notmuch
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula j...@nikula.org wrote:
 On Thu, 17 May 2012, Jameson Graef Rollins jroll...@finestructure.net wrote:
 This makes sure it has proper initialization values when it's created.

 Please don't do this. It's unnecessary; if one field is initialized with
 a designated initializer, the rest are initialized to zero (or NULL).

It may be technically unnecessary, but why is that a reason to not do
it?  I intentionally did this to make it clear what the defaults are.
Otherwise the defaults are essentially undefined, which is not good.
Maybe the structure initializes to the correct defaults, but why count
on that when we can set them to the correct default, and have it clear
to readers of the code?

jamie.


pgpX1NFOEAHqa.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On May 17, 2012 5:26 PM, Jameson Graef Rollins jroll...@finestructure.net
wrote:

 On Thu, May 17 2012, Jani Nikula j...@nikula.org wrote:
  On Thu, 17 May 2012, Jameson Graef Rollins jroll...@finestructure.net
wrote:
  This makes sure it has proper initialization values when it's created.
 
  Please don't do this. It's unnecessary; if one field is initialized with
  a designated initializer, the rest are initialized to zero (or NULL).

 It may be technically unnecessary, but why is that a reason to not do
 it?  I intentionally did this to make it clear what the defaults are.
 Otherwise the defaults are essentially undefined, which is not good.
 Maybe the structure initializes to the correct defaults, but why count
 on that when we can set them to the correct default, and have it clear
 to readers of the code?

The values are not undefined, they are properly initialized, and we can
count on it. For sure, not maybe. If you want to explicitly set them for
clarity, it's a matter of taste. Personally I find it too verbose, but then
again notmuch code is generally fairly verbose. If you insist on it, please
at least drop the extra temp crypto variable, and initialize the struct in
one initializer.

BR,
Jani.


 jamie.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula j...@nikula.org wrote:
 The values are not undefined, they are properly initialized, and we can
 count on it. For sure, not maybe. If you want to explicitly set them for
 clarity, it's a matter of taste. Personally I find it too verbose, but then
 again notmuch code is generally fairly verbose.

I want them explicitly set for clarity, as well as safety.  Code is
meant to be read by humans, not computers.  Brevity is not always a
virtue if it sacrifices clarity.  It's much nicer to have the defaults
clearly stated in the initialization, than to force the reader to
understand how the initialization works and to interpret what that means
for the current case.  I also don't think it's safe to assume that the
variables will be always be properly initialized in your favor in
perpetuity.  It's much safer to explicitly set them to what you want
them to be rather than just assume they'll be set correctly.

 If you insist on it, please at least drop the extra temp crypto
 variable, and initialize the struct in one initializer.

I don't see why this matters either.  Again, I think this is just a
matter of taste.  I would rather the code be verbose where clarity
requires it, rather than always trying to make the code as terse as
possible.

jamie.


pgp2cWfT5P1Tb.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jani Nikula]

On Thu, 17 May 2012, Jameson Graef Rollins jroll...@finestructure.net wrote:
 On Thu, May 17 2012, Jani Nikula j...@nikula.org wrote:
 The values are not undefined, they are properly initialized, and we can
 count on it. For sure, not maybe. If you want to explicitly set them for
 clarity, it's a matter of taste. Personally I find it too verbose, but then
 again notmuch code is generally fairly verbose.

 I want them explicitly set for clarity, as well as safety.  Code is
 meant to be read by humans, not computers.  Brevity is not always a
 virtue if it sacrifices clarity.  It's much nicer to have the defaults
 clearly stated in the initialization, than to force the reader to
 understand how the initialization works and to interpret what that means
 for the current case.  I also don't think it's safe to assume that the
 variables will be always be properly initialized in your favor in
 perpetuity.  It's much safer to explicitly set them to what you want
 them to be rather than just assume they'll be set correctly.

In short, when you read enough code, having everything explicitly stated
becomes a burden. It's explicit, but you have to read it all, even when
there really is no need to.

 If you insist on it, please at least drop the extra temp crypto
 variable, and initialize the struct in one initializer.

 I don't see why this matters either.  Again, I think this is just a
 matter of taste.  I would rather the code be verbose where clarity
 requires it, rather than always trying to make the code as terse as
 possible.

You introduce an extra variable that every reader of your code has to
track down to realize that it's only ever used once to initialize
another variable. You make code harder for other people to read.

I have now offered my review and opinions on the matter; I will not
pursue this discussion further.


BR,
Jani.
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

On Thu, May 17 2012, Jani Nikula j...@nikula.org wrote:
 In short, when you read enough code, having everything explicitly
 stated becomes a burden. It's explicit, but you have to read it all,
 even when there really is no need to.

Not everyone who reads the code is an expert.  I think it's important
for the code to be clear to everyone.  Experts can more easily gloss
over the trivial parts than novices can divine things that are hidden.

jamie.


pgp3Un0fY8kTJ.pgp
Description: PGP signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Daniel Kahn Gillmor]

On 05/17/2012 12:45 PM, Jameson Graef Rollins wrote:
 I want them explicitly set for clarity, as well as safety.  Code is
 meant to be read by humans, not computers.  

I sympathize with this sentiment.

 It's much safer to explicitly set them to what you want
 them to be rather than just assume they'll be set correctly.

I don't think it's an assumption -- Jani is probably relying on the C
standard. Consider, for example, C99 [0]'s section 6.7.8.19, which says:

  all subobjects that are not initialized explicitly shall be
  initialized implicitly the same as objects that have static
  storage duration.

the latter clause references 6.7.8.10, which says:

   If an object that has static storage duration is not
   initialized explicitly, then:
 — if it has pointer type, it is initialized to a null pointer;
 — if it has arithmetic type, it is initialized to (positive or
   unsigned) zero;
 — if it is an aggregate, every member is initialized
   (recursively) according to these rules;

So it's not just an assumption, it's a guarantee from the underlying
language standard.

That said, it's a guarantee i was unaware of until i researched this.
I'm certainly not a C guru, but i've internalized a fair amount of C's
rules and structure and i'd never heard of this
subobject-default-initialization-when-other-subobjects-are-initialized
rule before.   If i'd seen the uninitialized members of the struct, and
that they were being used without explicit initialization, i would have
had to do a bit of digging to understand what's happening.

The real tradeoff in this choice is whether we prefer:

 a) more compact code to facilitate quick reading by experts

   or

 b) more verbose code to facilitate comprehension by the non-expert.

I started this discussion leaning strongly toward the (b) perspective.
But now that i know the relevant bits of the standard, i can sympathize
with the (a) perspective as well :P

Overall, i think i'm still in the (b) camp.  But i think it's more
important that we don't allow dithering over this issue to prevent the
inclusion of this patch series, which is a step in the right direction
for handling S/MIME messages as well as PGP/MIME.

--dkg

PS gcc's -pedantic argument provides the following warning:

 error: ISO C90 forbids specifying subobject to initialize

So we probably want to specify -std=c99 at least to ensure our choice of
subobject initialization is respected.

[0] http://www.open-std.org/jtc1/sc22/WG14/www/docs/n1256.pdf



signature.asc
Description: OpenPGP digital signature
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

Re: [PATCH 4/6] cli: intialize crypto structure in show and reply

[Austin Clements]

Quoth Jameson Graef Rollins on May 16 at  2:55 pm:
 This makes sure it has proper initialization values when it's created.
 ---
  notmuch-reply.c |5 -
  notmuch-show.c  |   10 +-
  2 files changed, 13 insertions(+), 2 deletions(-)
 
 diff --git a/notmuch-reply.c b/notmuch-reply.c
 index 6662adb..3c967a0 100644
 --- a/notmuch-reply.c
 +++ b/notmuch-reply.c
 @@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
  char *query_string;
  int opt_index, ret = 0;
  int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
 notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
 -notmuch_crypto_t crypto = { .decrypt = FALSE };
 +notmuch_crypto_t crypto = {
 + .decrypt = FALSE,
 + .gpgctx = NULL,
 +};
  int format = FORMAT_DEFAULT;
  int reply_all = TRUE;
  
 diff --git a/notmuch-show.c b/notmuch-show.c
 index 8b4d308..c606333 100644
 --- a/notmuch-show.c
 +++ b/notmuch-show.c
 @@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), 
 unused (char *argv[]))
  char *query_string;
  int opt_index, ret;
  const notmuch_show_format_t *format = format_text;
 -notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
 +notmuch_crypto_t crypto = {
 + .decrypt = FALSE,
 + .gpgctx = NULL,
 +};
 +notmuch_show_params_t params = {
 + .part = -1,
 + .omit_excluded = TRUE,
 + .crypto = crypto,
 +};

You can omit the temporary variable and avoid the struct copy by doing
something like this:

notmuch_show_params_t params = {
.part = -1,
.omit_excluded = TRUE,
.crypto = {
.decrypt = FALSE,
.gpgctx = NULL,
},
};

  int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
  notmuch_bool_t verify = FALSE;
  int exclude = EXCLUDE_TRUE;
___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

This makes sure it has proper initialization values when it's created.
---
 notmuch-reply.c |5 -
 notmuch-show.c  |   10 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 6662adb..3c967a0 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 char *query_string;
 int opt_index, ret = 0;
 int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
-notmuch_crypto_t crypto = { .decrypt = FALSE };
+notmuch_crypto_t crypto = {
+   .decrypt = FALSE,
+   .gpgctx = NULL,
+};
 int format = FORMAT_DEFAULT;
 int reply_all = TRUE;

diff --git a/notmuch-show.c b/notmuch-show.c
index 8b4d308..c606333 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused 
(char *argv[]))
 char *query_string;
 int opt_index, ret;
 const notmuch_show_format_t *format = _text;
-notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
+notmuch_crypto_t crypto = {
+   .decrypt = FALSE,
+   .gpgctx = NULL,
+};
+notmuch_show_params_t params = {
+   .part = -1,
+   .omit_excluded = TRUE,
+   .crypto = crypto,
+};
 int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
 notmuch_bool_t verify = FALSE;
 int exclude = EXCLUDE_TRUE;
-- 
1.7.10

[PATCH 4/6] cli: intialize crypto structure in show and reply

[Jameson Graef Rollins]

This makes sure it has proper initialization values when it's created.
---
 notmuch-reply.c |5 -
 notmuch-show.c  |   10 +-
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/notmuch-reply.c b/notmuch-reply.c
index 6662adb..3c967a0 100644
--- a/notmuch-reply.c
+++ b/notmuch-reply.c
@@ -673,7 +673,10 @@ notmuch_reply_command (void *ctx, int argc, char *argv[])
 char *query_string;
 int opt_index, ret = 0;
 int (*reply_format_func)(void *ctx, notmuch_config_t *config, 
notmuch_query_t *query, notmuch_crypto_t *crypto, notmuch_bool_t reply_all);
-notmuch_crypto_t crypto = { .decrypt = FALSE };
+notmuch_crypto_t crypto = {
+   .decrypt = FALSE,
+   .gpgctx = NULL,
+};
 int format = FORMAT_DEFAULT;
 int reply_all = TRUE;
 
diff --git a/notmuch-show.c b/notmuch-show.c
index 8b4d308..c606333 100644
--- a/notmuch-show.c
+++ b/notmuch-show.c
@@ -983,7 +983,15 @@ notmuch_show_command (void *ctx, unused (int argc), unused 
(char *argv[]))
 char *query_string;
 int opt_index, ret;
 const notmuch_show_format_t *format = format_text;
-notmuch_show_params_t params = { .part = -1, .omit_excluded = TRUE };
+notmuch_crypto_t crypto = {
+   .decrypt = FALSE,
+   .gpgctx = NULL,
+};
+notmuch_show_params_t params = {
+   .part = -1,
+   .omit_excluded = TRUE,
+   .crypto = crypto,
+};
 int format_sel = NOTMUCH_FORMAT_NOT_SPECIFIED;
 notmuch_bool_t verify = FALSE;
 int exclude = EXCLUDE_TRUE;
-- 
1.7.10

___
notmuch mailing list
notmuch@notmuchmail.org
http://notmuchmail.org/mailman/listinfo/notmuch