Re: mapi32: Write-strings warnings fix

[Peter Oberndorfer]

On Saturday 03 June 2006 16:33, Andrew Talbot wrote:
> Changelog:
>     mapi32: Write-strings warnings fix.
>
> diff -urN a/dlls/mapi32/sendmail.c b/dlls/mapi32/sendmail.c
> --- a/dlls/mapi32/sendmail.c	2006-05-23 17:24:40.000000000 +0100
> +++ b/dlls/mapi32/sendmail.c	2006-06-03 14:44:21.000000000 +0100
> @@ -113,8 +113,8 @@
>      }
>      if (message->nFileCount) FIXME("Ignoring attachments\n");
>
> -    subject = message->lpszSubject ? message->lpszSubject : "";
> -    body = message->lpszNoteText ? message->lpszNoteText : "";
> +    subject = message->lpszSubject ? message->lpszSubject : NULL;
> +    body = message->lpszNoteText ? message->lpszNoteText : NULL;
> I think this is wrong, as it makes the whole "? :" construct unnecessary.
> (for non NULL it evaluates to "message->lpszNoteText" and for NULL it becomes NULL;
>
> TRACE( "Subject: %s\n", debugstr_a(subject) );
>      TRACE( "Body: %s\n", debugstr_a(body) );

Greetings Peter

mapi32: Write-strings warnings fix

[Andrew Talbot]

Peter Oberndorfer wrote:
> On Saturday 03 June 2006 16:33, Andrew Talbot wrote:
> > Changelog:
> >     mapi32: Write-strings warnings fix.
> >
> > diff -urN a/dlls/mapi32/sendmail.c b/dlls/mapi32/sendmail.c
> > --- a/dlls/mapi32/sendmail.c 2006-05-23 17:24:40.000000000 +0100
> > +++ b/dlls/mapi32/sendmail.c 2006-06-03 14:44:21.000000000 +0100
> > @@ -113,8 +113,8 @@
> >      }
> >      if (message->nFileCount) FIXME("Ignoring attachments\n");
> >
> > -    subject = message->lpszSubject ? message->lpszSubject : "";
> > -    body = message->lpszNoteText ? message->lpszNoteText : "";
> > +    subject = message->lpszSubject ? message->lpszSubject : NULL;
> > +    body = message->lpszNoteText ? message->lpszNoteText : NULL;
> > I think this is wrong, as it makes the whole "? :" construct unnecessary.
> > (for non NULL it evaluates to "message->lpszNoteText" and for NULL it
> > becomes NULL;
>
> Greetings Peter

Hi Peter,

I believe you are right! I started by trying to prevent the assignment of an
empty string constant to a non-const pointer to char, looked up the
MapiMessage struc specification and noted that an unsupplied value of
lpszSubject or lpszNoteText could otherwise be represented by NULL, saving
me a small amount of work. But I failed to observe that the constructs then
simplify to

    subject = message->lpszSubject;
    body = message->lpszNoteText;

I shall submit an amended version of the patch, accordingly. Well-spotted
and thanks!

-- Andy.

mapi32: Write-strings warnings fix

[Hans Leidekker]

On Saturday 03 June 2006 21:26, Andrew Talbot wrote:
> subject = message->lpszSubject;
>     body = message->lpszNoteText;

That won't do either because these pointers are dereferenced a few lines
further down. How about this patch?
 
-    char *address = "", *to = NULL, *cc = NULL, *bcc = NULL, *subject, *body;
+    char *to = NULL, *cc = NULL, *bcc = NULL;
+    const char *subject, *body, *address = "";

 -Hans

mapi32: Write-strings warnings fix

[Andrew Talbot]

Hans Leidekker wrote:
> That won't do either because these pointers are dereferenced a few lines
> further down. How about this patch?
>
> -    char *address = "", *to = NULL, *cc = NULL, *bcc = NULL, *subject,
> *body;
> +    char *to = NULL, *cc = NULL, *bcc = NULL;
> +    const char *subject, *body, *address = "";
>
> -Hans

The problem is that the SDK defines the strings as non-const, so making
subject and body const char * would cause a qual-cast violation.

I think there are two solutions:

1) I could put casts in the sprintf() line, thus:

    sprintf( mailto, format, to ? to : "", (char *) subject, cc ? cc : "",                
bcc ? bcc : "", (char *) body );

or

2) I could define

    const CHAR empty[] = "";

and reinstate the original ?: construct. I'm not sure which is best, if
either.

-- Andy.

mapi32: Write-strings warnings fix

[Andrew Talbot]

Option 2 is the way to go.

-- A.

mapi32: Write-strings warnings fix

[Andrew Talbot]

Actually, sorry, my earlier statement was rubbish. But the point of the
Write-strings warning is to flag the discarding of a qualifer. Thus

    char *s = "string"; /* bad */

discards the effective const factor of the string constant. So I can't
assign const char * values to char * variables.

-- Andy.

mapi32: Write-strings warnings fix

[Hans Leidekker]

On Saturday 03 June 2006 22:06, Andrew Talbot wrote:
> The problem is that the SDK defines the strings as non-const, so making
> subject and body const char * would cause a qual-cast violation.

I see no warnings with -Wwrite-strings and -Wcast-qual.

 -Hans

mapi32: Write-strings warnings fix

[Andrew Talbot]

Hans Leidekker wrote:
> I see no warnings with -Wwrite-strings and -Wcast-qual.
>
> -Hans

Hans,

Yes, you are right. Of course, the values are being taken from the SDK
elements, not being assigned to them, so the local variable can and should
be constified.

-- Andy.

mapi32: Write-strings warnings fix

[Andrew Talbot]

Hans Leidekker wrote:
> [...] How about this patch?
>
> -    char *address = "", *to = NULL, *cc = NULL, *bcc = NULL, *subject,
> *body;
> +    char *to = NULL, *cc = NULL, *bcc = NULL;
> +    const char *subject, *body, *address = "";
>
> -Hans

Because of the testing:

    if (!message) return MAPI_E_FAILURE;

and

        if (!message->lpRecips)
        {
            WARN("No recipients found\n");
            return MAPI_E_FAILURE;
        }

before

        address = message->lpRecips[i].lpszAddress;
        if (address)
        {

would it be safe to omit the initialization of "address"?

-- Andy.

mapi32: Write-strings warnings fix

[Hans Leidekker]

On Sunday 04 June 2006 00:27, Andrew Talbot wrote:
> address = message->lpRecips[i].lpszAddress;
>         if (address)
>         {
>
> would it be safe to omit the initialization of "address"?

Yes, but it shortens the lines below and improves readability of
the code.

 -Hans

mapi32: Write-strings warnings fix

[Andrew Talbot]

> > would it be safe to omit the initialization of "address"?
>
> Yes, but it shortens the lines below and improves readability of
> the code.
>
> -Hans

I am sorry to make such a meal of this, but - in addition to the
write-strings issue - I noticed that the original code initialized
"address" thus:

    char *address = "";

(which you correctly then constified).

What I am saying is, because of all the checking code which follows, can we
be confident that every message->lpRecips[i].lpszAddress must have gained a
value, and therefore just write

    const char *address;

for "address", without the empty-string initial assignment?

I shall post the patch, in essence, as you kindly stated, to supersede my
earlier offerings before Alexandre gets a chance to look at my mess. But it
would be nice to know the answer to the above. Again - and without much
thinking - if "address" does need to be set to a default value, should it
be to empty-string, or to NULL?

-- Andy.

mapi32: Write-strings warnings fix

[Hans Leidekker]

On Sunday 04 June 2006 11:45, Andrew Talbot wrote:
> I am sorry to make such a meal of this

Don't be sorry. I like your persistence.
> What I am saying is, because of all the checking code which follows, can we
> be confident that every message->lpRecips[i].lpszAddress must have gained a
> value, and therefore just write
>
> const char *address;
>
> for "address", without the empty-string initial assignment?

I see what you mean now, yes address doesn't have to be initialised, there's
no way it could be used uninitialised in this code.

 -Hans

mapi32: Write-strings warnings fix

[Andrew Talbot]

Hans Leidekker wrote:
> Don't be sorry. I like your persistence.

And I appreciate your kindness, thank you.
> I see what you mean now, yes address doesn't have to be initialised,
> there's no way it could be used uninitialised in this code.
>
> -Hans

I shall let the dust settle, then probably put in another patch to remove
the superfluous initialisation.

Thanks,

-- Andy.