[webkit-reviews] review denied: [Bug 104613] [WK2] Fix memory sampler for ARM Linux : [Attachment 178689] 2nd try

[bugzilla-daemon at webkit.org]

Benjamin Poulain <benjamin at webkit.org> has denied Laszlo Gombos
<laszlo.gombos at webkit.org>'s request for review:
Bug 104613: [WK2] Fix memory sampler for ARM Linux
https://bugs.webkit.org/show_bug.cgi?id=104613

Attachment 178689: 2nd try
https://bugs.webkit.org/attachment.cgi?id=178689&action=review

------- Additional Comments from Benjamin Poulain <benjamin at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=178689&action=review


> Source/WebKit2/ChangeLog:11
> +
> +	   Make sure that the return value for fgetc() is first assigned to a
> +	   signed variable to check for EOF and avoid the potential for an
infinite
> +	   loop. This is important for ARM platforms where "char" type is
> +	   unsigned by default.

This whole signed-unsigned char is not the problem. The problem is the
assignment from fgetc() was a one with loss. It is incorrect, ARM or not.

> Source/WebKit2/Shared/linux/WebMemorySamplerLinux.cpp:70
> +    int ch;
> +    while ((index < maxBuffer) && (ch = fgetc(file) != EOF)) {
> +	   char charCode = ch;
> +	   if (isASCIISpace(charCode) && index) // Break on non-initial ASCII
space.

This code is harder to read than it should.

Please split your condition and assignment to make something nice and clean.
You probably also want a c++ cast for int->char to make it explicit.