Re: comctl32_tests: Janitorial: write-strings warning fix

[Vitaliy Margolen]

Saturday, May 27, 2006, 11:57:33 AM, Andrew Talbot wrote:
> Changelog:
>     comctl32_tests: Janitorial: write-strings warning fix.
> diff -urN a/dlls/comctl32/tests/tab.c b/dlls/comctl32/tests/tab.c
> --- a/dlls/comctl32/tests/tab.c	2006-05-27 17:25:57.000000000 +0100
> +++ b/dlls/comctl32/tests/tab.c	2006-05-27 18:45:43.000000000 +0100
> @@ -57,6 +57,9 @@
>  {
>      HWND handle;
>      TCITEM tcNewTab;
> +    static char text1[] = "Tab 1",
> +    text2[] = "Wide Tab 2",
> +    text3[] = "T 3";
> handle = CreateWindow (
>   WC_TABCONTROLA,
> @@ -71,13 +74,13 @@
>      SendMessage (handle, WM_SETFONT, 0, (LPARAM) hFont);

> tcNewTab.mask = mask;
> -    tcNewTab.pszText = "Tab 1";
> +    tcNewTab.pszText = text1;
>      tcNewTab.iImage = 0;
>      SendMessage (handle, TCM_INSERTITEM, 0, (LPARAM) &tcNewTab);
> -    tcNewTab.pszText = "Wide Tab 2";
> +    tcNewTab.pszText = text2;
>      tcNewTab.iImage = 1;
>      SendMessage (handle, TCM_INSERTITEM, 1, (LPARAM) &tcNewTab);
> -    tcNewTab.pszText = "T 3";
> +    tcNewTab.pszText = text3;
>      tcNewTab.iImage = 2;
>      SendMessage (handle, TCM_INSERTITEM, 2, (LPARAM) &tcNewTab);

What warning are you talking about here? It's ascii constant text.
It should not be ever written to. Nor I ever seen a single warning from
this code.

Vitaliy Margolen

comctl32_tests: Janitorial: write-strings warning fix

[Andrew Talbot]

Hi Vitaliy,

I am having a go at the janitorial project of fixing certain compiler
warnings suggested at http://wiki.winehq.org/CompilerWarnings/.

When the -Wwrite-strings flag is set, code like this:

    char *a = "string";

produces the message "warning: assignment discards qualifiers from pointer
target type". Similarly, this:

    char *p = NULL;
    *p = "string";

produces the message "warning: initialization discards qualifiers from
pointer target type".

For internal simple string variables, the solution is to add the const
keywork:

   const char *a = "string";

However, with structS , because it seems unwise to change elements of type
char * to type const char *, I am changing the initializations or
assignments to satisfy the pre-existing type. Thus, I have used code like

    char s[] = "string";
    x.pszText = s;    

where x is the struct in question.

Because I have had at least one patch like this accepted, I assumed what I
was doing was OK. I guess another way of preventing this warning would be
to use:

    x.pszText = (LPSTR) "string";

Assuming there is no danger in using this method, then it is neater for a
single assignment. The array method may be better when the same text is
assigned several times in the same function or file.

If what I am doing is incorrect or not helpful, I would welcome advice to
that effect. After all, I'm trying to make myself useful, not mess things
up.

Thanks,

-- Andy.

comctl32_tests: Janitorial: write-strings warning fix

[Robert Shearman]

Andrew Talbot wrote:
> Because I have had at least one patch like this accepted, I assumed what I
> was doing was OK. I guess another way of preventing this warning would be
> to use:
>
> x.pszText = (LPSTR) "string";
>
> Assuming there is no danger in using this method, then it is neater for a
> single assignment. The array method may be better when the same text is
> assigned several times in the same function or file.

This method should be safe for this particular instance.
> If what I am doing is incorrect or not helpful, I would welcome advice to
> that effect. After all, I'm trying to make myself useful, not mess things
> up.

Absolutely. We should avoid regressions at all costs, but making literal 
strings non-writable should help to uncover bugs.

-- 
Rob Shearman

comctl32_tests: Janitorial: write-strings warning fix

[Andrew Talbot]

Robert Shearman wrote:
> Absolutely. We should avoid regressions at all costs, but making literal
> strings non-writable should help to uncover bugs.

Of course, what I am doing with the structure members is to say "These have
been declared as writeable (LPSTR pszXYZ); I don't know whether they are
meant to be writeable, so I dare not change them to LPCSTR pcszXYZ. In
which case, I am cutting the compiler noise when looking for opportunities
to declare suitable strings const (using the -Wwrite-strings switch). The
question is: is this wise, or am I masking potential bugs?

comctl32_tests: Janitorial: write-strings warning fix

[Andrew Talbot]

On reflection, I think I need to look at constifying the structure members,
where possible, rather than unconstifying the values that are fed to them.

-- Andy.

comctl32_tests: Janitorial: write-strings warning fix

[Vitaliy Margolen]

Sunday, May 28, 2006, 12:19:30 PM, Andrew Talbot wrote:
> On reflection, I think I need to look at constifying the structure members,
> where possible, rather than unconstifying the values that are fed to them.

Exactly. Except that's the part that we can't do nothing about - that's the
way they are in SDK.

Vitaliy

comctl32_tests: Janitorial: write-strings warning fix

[Francois Gouget]

On Sun, 28 May 2006, Vitaliy Margolen wrote:
> Sunday, May 28, 2006, 12:19:30 PM, Andrew Talbot wrote:
> > On reflection, I think I need to look at constifying the structure members,
> > where possible, rather than unconstifying the values that are fed to them.
>
> Exactly. Except that's the part that we can't do nothing about - that's the
> way they are in SDK.

Except the latest versions of the SDK, the Vista one in particular, have 
improved const-correctness, so it could be worth checking whether they 
changes the struct declaration on a case by case basis.

-- 
Francois Gouget <fgouget(a)free.fr>              http://fgouget.free.fr/
                   Dieu dit: "M-x Lumière". Et la lumière fut.