Re: WINE_UNICODE_TEXT

[Francois Gouget]

On Mon, 29 Jan 2001, Jon Griffiths wrote:
> Hi,
>
> This patch removes the warnings from the WINE_UNICODE_TEXT macro and fixes an 
> obscure (read: uninvestgated) bug where the value of WCHAR('x') + 1 is off by

You mean WINE_UNICODE_TEXT('x')+1 != WINE_UNICODE_TEXT('y') ?
  I see why this could happen: gcc considers that the expression is of
type WCHAR* and since sizeof(WCHAR)==2, it just adds two.
> one. Currently 2-3 warnings are generated whenever TEXT() is used in Winelib 
> programs, depending on whether a string or char is the target.
>
> Only one warning remains, when using WCHAR('x') the result should be 
> explicitly cast to WCHAR to get rid of it. If you do this -Wall is blissfully 
> silent once again...

Which means you get warnings too. I'm not too convinced about this
patch especially because it does not return the proper type. On the
other hand it seems what I did returns a WCHAR* (after a fashion). But
it seems there's no really good solution (besides -fshort-wchar) :-(.

If you are going to modify your source, I think casting TEXT('x') to
WCHAR is not the solution. The solution is to not use TEXT (and
equivalents) on characters.

--
Francois Gouget         [e-mail]        http://fgouget.free.fr/
                Linux: It is now safe to turn on your computer.

WINE_UNICODE_TEXT

[Francois Gouget]

On Thu, 1 Feb 2001, Francois Gouget wrote:
> On Mon, 29 Jan 2001, Jon Griffiths wrote:
> > Hi,
> >
> > This patch removes the warnings from the WINE_UNICODE_TEXT macro and fixes an 
> > obscure (read: uninvestgated) bug where the value of WCHAR('x') + 1 is off by
>
> You mean WINE_UNICODE_TEXT('x')+1 != WINE_UNICODE_TEXT('y') ?
>   I see why this could happen: gcc considers that the expression is of
> type WCHAR* and since sizeof(WCHAR)==2, it just adds two.

Actually I suspect there's another problem with your patch. since you
cast the return values to void*, WINE_UNICODE_TEXT("abc")+1 will very
likely be wrong. Also I believe void* arithmetic is a gcc extension.

Do you have a lot of places where your application uses
WINE_UNICODE_TEXT with a character? I just know of a <10 places in the
MFC. Alexandre suggests we just handle the string case and let it fail
at compile time if it is used with a char. It may be better than
compiling fine but doing incorrect things.

--
Francois Gouget         [e-mail]        http://fgouget.free.fr/
  Any sufficiently advanced Operating System is indistinguishable from Linux

WINE_UNICODE_TEXT

[Jon Griffiths]

Hi,
> Actually I suspect there's another problem with your patch. since you
> cast the return values to void*, WINE_UNICODE_TEXT("abc")+1 will very
> likely be wrong. Also I believe void* arithmetic is a gcc extension.

I haven't checked, but youre probably right. However Ive never come across 
that usage of  a literal in unicode or ascii (why increment a string lteral 
at declaration time?). But doing it to chars is quite common e.g. converting 
to a number string ('a' + n etc).
> Do you have a lot of places where your application uses
> WINE_UNICODE_TEXT with a character? I just know of a <10 places in the
> MFC.

Only about 5 cases in VWCL itself.
> Alexandre suggests we just handle the string case and let it fail
> at compile time if it is used with a char. It may be better than
> compiling fine but doing incorrect things.

Either works for me. All I really want is -Wall clean ;-)

Cheers,
Jon

WINE_UNICODE_TEXT

[Jon Griffiths]

Hi,
> type WCHAR* and since sizeof(WCHAR)==2, it just adds two.

Sounds likely. A cast back to WCHAR fixes it, anyhow, but isn't 
especially nice.
> Which means you get warnings too. I'm not too convinced about this
> patch especially because it does not return the proper type.
> On the
> other hand it seems what I did returns a WCHAR* (after a fashion).

As you say, based on the above it seems that currently the conditional 
expression returns WCHAR* despite their being 2 possible returns. So it 
returns the wrong type for characters anyway.

The whole macro can be cast to a WCHAR* if thats a problem and the behavior 
will be the same as with the patch as it stands (only one warning, when used 
with L'x' , for assigning a pointer to an int), but the bug above will 
remain. Assigning from a void* is perfectly safe and fixes the bug, so I 
chose that.
> it seems there's no really good solution (besides -fshort-wchar) :-(.

Sadly we're a long way away from having that option for the majority.Also 
going forward with Winelib there will be people who aren't using gcc, and who 
mightn't have that option at all.
> If you are going to modify your source, I think casting TEXT('x') to
> WCHAR is not the solution. The solution is to not use TEXT (and
> equivalents) on characters.

I agree, its not the best solution. My point was that it is possible to fix 
the problem (either by casting or some other method), and then compile your 
code without warnings.

The purpose of the change is to remove all warnings except for the specific 
problematic case, which makes it considerably easier to locate and fix the 
offending code. Currently you get spurious warnings which dont help when the 
macro is used correctly (with L'string'). Given that it generates multiple 
warnings even when used corectly, how is a developer to notice that s/he has 
to fix that one case?

IMHO its better to only generate a warning for the case that has problems, 
especially when its safe to eliminate the other warnings.

So rather than leaving the broken macro in with warnings for both cases and a 
bug for one case, why not just remove support for characters from the macro 
altogether? Personally I prefer the patch because IMO it leaves the option to 
ignore the warning, and fixes the bug as well.

Cheers,
Jon