On 02/17/2016 10:27 AM, Charles Davis wrote:
Clang warns about this abs(3) call because the argument is of unsigned type. Apparently, though, this is actually a signed value that just happens to be stored in a DWORD. (Then why is this parameter to ExtCreatePen() a DWORD?)
In any case, quiesce this warning with a cast.
Well, clang is wrong here. abs() takes a signed integer and the implicit cast from unsigned to signed is perfectly valid C.
The policy in Wine is to not uglify the code just to silence misguided static analyzers. And other static code checkers might complain about the superfluous and unneeded explicit cast (I'm working on and off on that).
bye michael
Signed-off-by: Charles Davis cdavis5x@gmail.com --- dlls/gdi32/pen.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/dlls/gdi32/pen.c b/dlls/gdi32/pen.c index 2ae2f2b..cf4b080 100644 --- a/dlls/gdi32/pen.c +++ b/dlls/gdi32/pen.c @@ -195,7 +195,7 @@ HPEN WINAPI ExtCreatePen( DWORD style, DWORD width, if (logbrush.lbStyle == BS_DIBPATTERN) logbrush.lbStyle = BS_DIBPATTERNPT;
penPtr->logpen.elpPenStyle = style; - penPtr->logpen.elpWidth = abs(width); + penPtr->logpen.elpWidth = abs((int)width); penPtr->logpen.elpBrushStyle = logbrush.lbStyle; penPtr->logpen.elpColor = logbrush.lbColor; penPtr->logpen.elpHatch = brush->lbHatch;
On Feb 17, 2016, at 4:12 AM, Michael Stefaniuc mstefani@redhat.com wrote:
On 02/17/2016 10:27 AM, Charles Davis wrote:
Clang warns about this abs(3) call because the argument is of unsigned type. Apparently, though, this is actually a signed value that just happens to be stored in a DWORD. (Then why is this parameter to ExtCreatePen() a DWORD?)
In any case, quiesce this warning with a cast.
Well, clang is wrong here. abs() takes a signed integer and the implicit cast from unsigned to signed is perfectly valid C.
Warnings are not for calling out things that are invalid — that's what errors are for — they're for calling out things that may be unintentional or confusing. I think this qualifies. It's fairly non-obvious that this DWORD value should be treated as signed, as evidenced by Chip's initial approach of just removing the abs() call. Something is needed to clarify the intent of the code.
Regards, Ken
On 02/17/2016 04:04 PM, Ken Thomases wrote:
On Feb 17, 2016, at 4:12 AM, Michael Stefaniuc mstefani@redhat.com wrote:
On 02/17/2016 10:27 AM, Charles Davis wrote:
Clang warns about this abs(3) call because the argument is of unsigned type. Apparently, though, this is actually a signed value that just happens to be stored in a DWORD. (Then why is this parameter to ExtCreatePen() a DWORD?)
In any case, quiesce this warning with a cast.
Well, clang is wrong here. abs() takes a signed integer and the implicit cast from unsigned to signed is perfectly valid C.
Warnings are not for calling out things that are invalid — that's what
There is a lot of undefined behavior for which the compilers don't output any errors or warnings. With valid C I meant well defined C.
errors are for — they're for calling out things that may be unintentional or confusing. I think this qualifies. It's fairly non-obvious that this DWORD value should be treated as signed, as evidenced by Chip's initial approach of just removing the abs() call.
No, he just removed it because clang complained, probably with a misleading warning. Doubt he would have done it on a code review. Because the abs() call as it is looks perfectly normal and does the right thing.
By adding the cast he just added a big red blinking "Warning there might be dragons". And there are no dragons, just a normal implicit unsigned to signed cast. No need to drag attention to it. In an ideal world one would fix the type of that parameter but we don't have that freedom in Wine.
Something is needed to clarify the intent of the code.
IMHO the intend of the code was clearer without the cast. But I guess this falls into "the matter of taste" category and Alexandre will apply it as Huw signed off on it.
bye michael
-
Ken Thomases -
Michael Stefaniuc