Fix two bugs found by a new clang warning I'm currently testing:

Fix two bugs found by a new clang warning I'm currently testing:

/Users/thakis/src/chrome-git/src/base/md5.cc:250:24: error: The sizeof expression in 'memset' has type 'struct Context *', the same type that the first argument has. The sizeof expression should probably have type 'struct Context' instead. [-Werror]
        memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
                       ^
1 error generated.


/Users/thakis/src/chrome-git/src/media/video/capture/fake_video_capture_device.cc:75:32: error: The sizeof expression in 'memset' has type 'unsigned char *', the same type that the first argument has. The sizeof expression should probably have type 'unsigned char' instead. [-Werror]
  memset(fake_frame_.get(), 0, sizeof(fake_frame_.get()));
                               ^
1 error generated.

The warning is up for review here: http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110613/042719.html

BUG=none
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=88855

[Evan Martin, 2011-06-13 17:15:00]

LGTM

Cool warning!  I wonder though what people who want to be warning
clean can do if the warning is misfiring for some reason.

On Sun, Jun 12, 2011 at 11:43 PM,  <thakis@chromium.org> wrote:
> Reviewers: Evan Martin, scherkus,
>
> Description:
> Fix two bugs found by a new clang warning I'm currently testing:
>
> /Users/thakis/src/chrome-git/src/base/md5.cc:250:24: error: The sizeof
> expression in 'memset' has type 'struct Context *', the same type that the
> first
> argument has. The sizeof expression should probably have type 'struct
> Context'
> instead. [-Werror]
>        memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
>                       ^
> 1 error generated.
>
>
>
/Users/thakis/src/chrome-git/src/media/video/capture/fake_video_capture_device.cc:75:32:
> error: The sizeof expression in 'memset' has type 'unsigned char *', the
> same
> type that the first argument has. The sizeof expression should probably have
> type 'unsigned char' instead. [-Werror]
>  memset(fake_frame_.get(), 0, sizeof(fake_frame_.get()));
>                               ^
> 1 error generated.
>
> The warning is up for review here:
>
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110613/042719.html
>
> BUG=none
> TEST=none
>
>
> Please review this at http://codereview.chromium.org/7003140/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>  M base/md5.cc
>  M media/video/capture/fake_video_capture_device.cc
>
>
> Index: base/md5.cc
> diff --git a/base/md5.cc b/base/md5.cc
> index
>
f85cf4390d954663662bb65d1637220d6bbb6d4a..fdda2a499c06dea8f6ed5bb023db4eb72af08e43
> 100644
> --- a/base/md5.cc
> +++ b/base/md5.cc
> @@ -247,7 +247,7 @@ void MD5Final(MD5Digest* digest, MD5Context *pCtx){
>         MD5Transform(ctx->buf, (uint32 *)ctx->in);
>         byteReverse((unsigned char *)ctx->buf, 4);
>         memcpy(digest->a, ctx->buf, 16);
> -        memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
> +        memset(ctx, 0, sizeof(*ctx));    /* In case it's sensitive */
>  }
>
>  std::string MD5DigestToBase16(const MD5Digest& digest){
> Index: media/video/capture/fake_video_capture_device.cc
> diff --git a/media/video/capture/fake_video_capture_device.cc
> b/media/video/capture/fake_video_capture_device.cc
> index
>
768271428d2f8c181ed0b20fdb9e994221b821b2..9de3a40d68fe2b050d214b1e723095c27a072a9f
> 100644
> --- a/media/video/capture/fake_video_capture_device.cc
> +++ b/media/video/capture/fake_video_capture_device.cc
> @@ -70,9 +70,10 @@ void FakeVideoCaptureDevice::Allocate(int width,
>     current_settings.frame_rate = 30;
>   }
>
> -  fake_frame_.reset(new uint8[current_settings.width *
> -                              current_settings.height * 3 / 2]);
> -  memset(fake_frame_.get(), 0, sizeof(fake_frame_.get()));
> +  size_t fake_frame_size =
> +      current_settings.width * current_settings.height * 3 / 2;
> +  fake_frame_.reset(new uint8[fake_frame_size]);
> +  memset(fake_frame_.get(), 0, fake_frame_size);
>
>   state_ = kAllocated;
>   observer_->OnFrameInfo(current_settings);
>
>
>

[Nico, 2011-06-13 17:19:29]

On Mon, Jun 13, 2011 at 10:14 AM, Evan Martin <evan@chromium.org> wrote:
> LGTM
>
> Cool warning!  I wonder though what people who want to be warning
> clean can do if the warning is misfiring for some reason.

Casting the pointer to void* explicitly makes it go away. Can you
think of a case where it might misfire? I couldn't, which is why I
don't output a note about this. I guess I should anyway :-)

Upstream suggested I make the warning warn more generally if the type
in sizeof isn't compatible with the pointee type in the 1st memset
argument so that it catches memset(ptr_to_a, 0, sizeof(B)) as well. I
can see that misfiring, so if I make this change I'll definitely make
the compiler write the void-cast note.

>
> On Sun, Jun 12, 2011 at 11:43 PM,  <thakis@chromium.org> wrote:
>> Reviewers: Evan Martin, scherkus,
>>
>> Description:
>> Fix two bugs found by a new clang warning I'm currently testing:
>>
>> /Users/thakis/src/chrome-git/src/base/md5.cc:250:24: error: The sizeof
>> expression in 'memset' has type 'struct Context *', the same type that the
>> first
>> argument has. The sizeof expression should probably have type 'struct
>> Context'
>> instead. [-Werror]
>>        memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
>>                       ^
>> 1 error generated.
>>
>>
>>
/Users/thakis/src/chrome-git/src/media/video/capture/fake_video_capture_device.cc:75:32:
>> error: The sizeof expression in 'memset' has type 'unsigned char *', the
>> same
>> type that the first argument has. The sizeof expression should probably have
>> type 'unsigned char' instead. [-Werror]
>>  memset(fake_frame_.get(), 0, sizeof(fake_frame_.get()));
>>                               ^
>> 1 error generated.
>>
>> The warning is up for review here:
>>
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20110613/042719.html
>>
>> BUG=none
>> TEST=none
>>
>>
>> Please review this at http://codereview.chromium.org/7003140/
>>
>> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>>
>> Affected files:
>>  M base/md5.cc
>>  M media/video/capture/fake_video_capture_device.cc
>>
>>
>> Index: base/md5.cc
>> diff --git a/base/md5.cc b/base/md5.cc
>> index
>>
f85cf4390d954663662bb65d1637220d6bbb6d4a..fdda2a499c06dea8f6ed5bb023db4eb72af08e43
>> 100644
>> --- a/base/md5.cc
>> +++ b/base/md5.cc
>> @@ -247,7 +247,7 @@ void MD5Final(MD5Digest* digest, MD5Context *pCtx){
>>         MD5Transform(ctx->buf, (uint32 *)ctx->in);
>>         byteReverse((unsigned char *)ctx->buf, 4);
>>         memcpy(digest->a, ctx->buf, 16);
>> -        memset(ctx, 0, sizeof(ctx));    /* In case it's sensitive */
>> +        memset(ctx, 0, sizeof(*ctx));    /* In case it's sensitive */
>>  }
>>
>>  std::string MD5DigestToBase16(const MD5Digest& digest){
>> Index: media/video/capture/fake_video_capture_device.cc
>> diff --git a/media/video/capture/fake_video_capture_device.cc
>> b/media/video/capture/fake_video_capture_device.cc
>> index
>>
768271428d2f8c181ed0b20fdb9e994221b821b2..9de3a40d68fe2b050d214b1e723095c27a072a9f
>> 100644
>> --- a/media/video/capture/fake_video_capture_device.cc
>> +++ b/media/video/capture/fake_video_capture_device.cc
>> @@ -70,9 +70,10 @@ void FakeVideoCaptureDevice::Allocate(int width,
>>     current_settings.frame_rate = 30;
>>   }
>>
>> -  fake_frame_.reset(new uint8[current_settings.width *
>> -                              current_settings.height * 3 / 2]);
>> -  memset(fake_frame_.get(), 0, sizeof(fake_frame_.get()));
>> +  size_t fake_frame_size =
>> +      current_settings.width * current_settings.height * 3 / 2;
>> +  fake_frame_.reset(new uint8[fake_frame_size]);
>> +  memset(fake_frame_.get(), 0, fake_frame_size);
>>
>>   state_ = kAllocated;
>>   observer_->OnFrameInfo(current_settings);
>>
>>
>>
>

[Ami GONE FROM WEBRTC_CHROMIUM, 2011-06-13 17:36:42]

>
> Can you think of a case where it might misfire?
>

If the test is for X & Y having the same type in a pattern like:
memset(X, _, sizeof(Y))
then it will mis-fire when X & Y are of array type
I.e. this is legit (if icky):
  char ar[6];
  memset(ar, 0, sizeof(ar));
because sizeof(ar)==6 (6.5.3.4.3 of C99).

-a

[Nico, 2011-06-13 17:57:41]

On 2011/06/13 17:36:42, fischman1 wrote:
> >
> > Can you think of a case where it might misfire?
> 
> If the test is for X & Y having the same type in a pattern like:
> memset(X, _, sizeof(Y))
> then it will mis-fire when X & Y are of array type
> I.e. this is legit (if icky):
>   char ar[6];
>   memset(ar, 0, sizeof(ar));
> because sizeof(ar)==6 (6.5.3.4.3 of C99).

I'm only warning if ar has pointer type (which arrays don't), so this case isn't
warned on.

(On the other hand void f(char ar[6]) {memset(a, 0, sizeof(a));} is warned on,
because arrays in parameter lists alway decay to pointers and the sizeof will
evaluation to 4 (sizeof char*) not, 6 in this case)

[Ami GONE FROM WEBRTC_CHROMIUM, 2011-06-13 18:08:30]

>
> I'm only warning if ar has pointer type (which arrays don't), so this case
> isn't
> warned on.
>

SGTM