Although I accept that my opinion may not be universally shared :-), I believe that it is better to turn -Wcast-qual on permanently and double-cast the appropriate return values of the relevant wide-string functions (strchrW(), strrchrW(), strpbrkW(), memchrW() and memrchrW()) than to leave it normally turned off: that way, the developers can avoid violations as they go, and give the janitors a break ;-). These functions are ubiquitous and widely understood, and they account for the vast majority of cast-qual warnings.
Because, as I understand it, we are aiming for C89 compatibility, and to try to maximise portability, I am proposing to use size_t, rather than uintptr_t (which is a C99 type): it seems like the next-best thing. So I would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { WCHAR *ret = NULL; do { if (*str == ch) ret = (WCHAR *)(size_t)str; } while (*str++); return ret; }
What do people think? Should I just send in the patch and see what criticism it engenders? Does anyone have a system on which this would fail or generate other warnings in place of the cast-qual one?
Needless to say, I hope to be tackling the cast-qual janitorial task, soon; hence, my sudden interest in this.
Thanks,
-- Andy.
Andrew Talbot wrote:
[...] So I would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) [...]
Yes, that was a typo: it should say "changes strrchrW()...".
-- Andy.
On Mon, Aug 07, 2006 at 09:54:20PM +0100, Andrew Talbot wrote:
would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { WCHAR *ret = NULL; do { if (*str == ch) ret = (WCHAR *)(size_t)str; } while (*str++); return ret; }
why not just have:
extern inline void *__deconst(const void *v) { return (char *)0 + ((const char *)v - (const char *)0)); }
Then the code above could be: extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { do { if (*str == ch) return __deconst(str); } while (*str++); return 0; }
David
David Laight wrote:
On Mon, Aug 07, 2006 at 09:54:20PM +0100, Andrew Talbot wrote:
would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { WCHAR *ret = NULL; do { if (*str == ch) ret = (WCHAR *)(size_t)str; } while (*str++); return ret; }
why not just have:
extern inline void *__deconst(const void *v) { return (char *)0 + ((const char *)v - (const char *)0)); }
Then the code above could be: extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { do { if (*str == ch) return __deconst(str); } while (*str++); return 0; }
David
That's interesting. IMHO, it's a bit more complex and obscure, but seems to ensure portability. I shall be interested to know what others think.
Thanks,
-- Andy.
Andrew Talbot wrote:
David Laight wrote:
On Mon, Aug 07, 2006 at 09:54:20PM +0100, Andrew Talbot wrote:
would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { WCHAR *ret = NULL; do { if (*str == ch) ret = (WCHAR *)(size_t)str; } while (*str++); return ret; }
why not just have:
extern inline void *__deconst(const void *v) { return (char *)0 + ((const char *)v - (const char *)0)); }
Then the code above could be: extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { do { if (*str == ch) return __deconst(str); } while (*str++); return 0; }
David
That's interesting. IMHO, it's a bit more complex and obscure, but seems to ensure portability. I shall be interested to know what others think.
what I don't like is that in order to plug a hole (casting from const foo* to foo*), we create a bigger hole by allowing to cast from const foo* to bar* (and the compiler will not give any warning) if we want to go into this, then I'd rather have _deconst explicitly use the type: #define __deconst(v,X) ({const X _v = (v); ((X)(int)_v);}) and in the previous example use __deconst(str, char*);
one could think of using gcc's typeof operator to avoid passing the type to deconst, but this doesn't seem possible (you have to express foo* in terms of const foo* which isn't possible AFAICT)
A+
On Tue, Aug 08, 2006 at 09:27:23PM +0200, Eric Pouech wrote:
what I don't like is that in order to plug a hole (casting from const foo* to foo*), we create a bigger hole by allowing to cast from const foo* to bar* (and the compiler will not give any warning) if we want to go into this, then I'd rather have _deconst explicitly use the type: #define __deconst(v,X) ({const X _v = (v); ((X)(int)_v);}) and in the previous example use __deconst(str, char*);
That isn't valid C, you could do: ((const X)(v) - (v), (X)(int)(v)) but that is likely to give a 'computed value not used' warning instead.
What you really don't want to do is allow casts from 'int' to 'foo *'. After all casting from 'foo *' to 'bar *' is easy.
David
David Laight wrote:
On Tue, Aug 08, 2006 at 09:27:23PM +0200, Eric Pouech wrote:
what I don't like is that in order to plug a hole (casting from const foo* to foo*), we create a bigger hole by allowing to cast from const foo* to bar* (and the compiler will not give any warning) if we want to go into this, then I'd rather have _deconst explicitly use the type: #define __deconst(v,X) ({const X _v = (v); ((X)(int)_v);}) and in the previous example use __deconst(str, char*);
That isn't valid C, you could do: ((const X)(v) - (v), (X)(int)(v)) but that is likely to give a 'computed value not used' warning instead.
What you really don't want to do is allow casts from 'int' to 'foo *'. After all casting from 'foo *' to 'bar *' is easy.
David
this is supported by gcc... of course we'd need to define differently the macro for gcc and other compilers but casting from foo* to bar* is (in most of the cases) a sign of bad design A+
Once qualifications are give, how hard it is to take them away, again!
Of course, if one were realising these small inline functions literally as inline code, one would not add a const qualification for a few lines, only to take it away again (with great difficulty).
Thus, one would not write:
static WCHAR s[] = {'h','e','l','l','o',0}; LPCWSTR ps1; /* [1] */ LPWSTR ps2; const WCHAR ch = 'h';
ps1 = s;
ps2 = NULL; do { if (*ps1 == ch) { ps2 = (LPWSTR)(uintptr_t)ps1; /* [2] */ break; } } while (*ps1++);
if (ps2 != NULL) *ps2 = 'y';
... when [1] need only be
LPWSTR;
and [2} need only be
ps2 = ps1;
So there might be some argument for specially declaring these few functions - suitably commented - as taking unqualified string parameters. (The problem with constifying both input and output pointers is that one might wish to use the returned pointer to modify the input string.)
However, now that the ANSI/ISO people have given use intptr_t and uintptr_t (in C99), I don't see what is wrong with, in effect, replacing
ret = (WCHAR *)str;
with
ret = (WCHAR *)(uintptr_t)str;
To avoid touching Wine's configuration files (to add [u]intptr_t for non-C99 systems) the intermediate cast might be have been (unsigned long). Maybe, (size_t) would have worked, and been more indicative of use(?) "unsigned long" should have met the needs of the platforms for which we cater. The spoiler, apparently, is that it won't work with Win64 type definitions. Though, I don't know the details.
Anyway, I shall look at fixing the straightforward cast-qual violations, first, and revisit the controversial issues until the end.
-- Andy.
On Mon, Aug 07, 2006 at 09:54:20PM +0100, Andrew Talbot wrote:
Although I accept that my opinion may not be universally shared :-), I believe that it is better to turn -Wcast-qual on permanently and double-cast the appropriate return values of the relevant wide-string functions (strchrW(), strrchrW(), strpbrkW(), memchrW() and memrchrW()) than to leave it normally turned off: that way, the developers can avoid violations as they go, and give the janitors a break ;-). These functions are ubiquitous and widely understood, and they account for the vast majority of cast-qual warnings.
Because, as I understand it, we are aiming for C89 compatibility, and to try to maximise portability, I am proposing to use size_t, rather than uintptr_t (which is a C99 type): it seems like the next-best thing. So I would like to submit a patch that, for example, changes strchrW() to:
extern inline WCHAR *strrchrW( const WCHAR *str, WCHAR ch ) { WCHAR *ret = NULL; do { if (*str == ch) ret = (WCHAR *)(size_t)str; } while (*str++); return ret; }
What do people think? Should I just send in the patch and see what criticism it engenders? Does anyone have a system on which this would fail or generate other warnings in place of the cast-qual one?
Needless to say, I hope to be tackling the cast-qual janitorial task, soon; hence, my sudden interest in this.
I asked our gcc gurus.
- If you want to cast, do not use size_t but uintptr_t.
Or:
- Do not inline the function, and compile its file without -Wcast-qual.
- Fix the prototype to read "extern inline const WCHAR *strrchrW( const WCHAR *str, WCHAR ch )"
Ciao, Marcus
Marcus Meissner wrote:
I asked our gcc gurus.
- If you want to cast, do not use size_t but uintptr_t.
Or:
Do not inline the function, and compile its file without -Wcast-qual.
Fix the prototype to read "extern inline const WCHAR *strrchrW( const
WCHAR *str, WCHAR ch )"
Ciao, Marcus
intptr_t and uintptr_t were, of course, introduced in C99 specifically to be integer types to which any valid pointer to an object may be converted. This is the ultimate way to go, I believe, but I would need help or advice to patch the appropriate make file to cater for users with non-C99 systems.
Incidentally, regarding the other suggested solutions: IMHO, I don't think it a good idea to remove the inlining, thus adding the overhead of a function call to these functions. And if one constifies the return value, then it becomes no longer possible to write code like:
WCHAR arrW[] = {'b','i','t',0}, *pwc;
if ((pwc = strrchrW(arrW, 'i')) != NULL) *pwc = 'a';
Thanks,
-- Andy.
-
Andrew Talbot -
David Laight -
Eric Pouech -
Marcus Meissner