Bug 116340

Summary: WebCore fails to compile with -Werror=maybe-uninitialized on GCC 4.8.0
Product: WebKit Reporter: Hugo Parente Lima <hugo.lima>
Component: WebCore Misc.Assignee: Hugo Parente Lima <hugo.lima>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, darin, kenneth, laszlo.gombos
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Hugo Parente Lima 2013-05-17 12:06:53 PDT
WebCore fails to compile with -Werror=maybe-uninitialized on GCC 4.8.0
Comment 1 Hugo Parente Lima 2013-05-17 12:10:14 PDT
Created attachment 202133 [details]
Patch
Comment 2 Chris Dumez 2013-05-17 12:27:30 PDT
Comment on attachment 202133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202133&action=review

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:157
>      RefPtr<StorageArea> storageArea = findStorageArea(0, storageId, frame);

Could we maybe initialize frame in findStorageArea() error cases instead? This would avoid double initialization in normal cases.
Comment 3 Hugo Parente Lima 2013-05-17 12:43:52 PDT
(In reply to comment #2)
> (From update of attachment 202133 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202133&action=review
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:157
> >      RefPtr<StorageArea> storageArea = findStorageArea(0, storageId, frame);
> 
> Could we maybe initialize frame in findStorageArea() error cases instead? This would avoid double initialization in normal cases.

Could be, let me see is g++ is smart enough.
Comment 4 Hugo Parente Lima 2013-05-17 12:47:38 PDT
(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 202133 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=202133&action=review
> > 
> > > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:157
> > >      RefPtr<StorageArea> storageArea = findStorageArea(0, storageId, frame);
> > 
> > Could we maybe initialize frame in findStorageArea() error cases instead? This would avoid double initialization in normal cases.
> 
> Could be, let me see is g++ is smart enough.

Hmmm, it isn't, findStorageArea() already does that:

line 237: targetFrame = frame;
Comment 5 Chris Dumez 2013-05-17 12:49:29 PDT
Comment on attachment 202133 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=202133&action=review

>>>> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:157
>>>>      RefPtr<StorageArea> storageArea = findStorageArea(0, storageId, frame);
>>> 
>>> Could we maybe initialize frame in findStorageArea() error cases instead? This would avoid double initialization in normal cases.
>> 
>> Could be, let me see is g++ is smart enough.
> 
> Hmmm, it isn't, findStorageArea() already does that:
> 
> line 237: targetFrame = frame;

No, it doesn't in the errors cases (when it returns 0). Which is why I assume g++ complains.
Comment 6 Benjamin Poulain 2013-05-17 13:51:33 PDT
Comment on attachment 202133 [details]
Patch

findStorageArea() looks like an awful mess :(
Can you fix that instead?
Comment 7 Hugo Parente Lima 2013-05-20 12:41:46 PDT
(In reply to comment #6)
> (From update of attachment 202133 [details])
> findStorageArea() looks like an awful mess :(
> Can you fix that instead?

Just a mess, awful mess is too much for a little piece of code :-).

Yes, this function could be refactored but the warning probably will be triggered anyway, so I'm start to think that the better way to fix it is to just disable this warning "maybe-uninitialized" due to the amount of false-positives generated by it.
Comment 8 Hugo Parente Lima 2013-05-21 15:49:51 PDT
Created attachment 202475 [details]
Patch
Comment 9 Chris Dumez 2013-05-21 22:45:00 PDT
Comment on attachment 202475 [details]
Patch

Looks fine. r=me.
Comment 10 WebKit Commit Bot 2013-05-22 06:44:51 PDT
Comment on attachment 202475 [details]
Patch

Clearing flags on attachment: 202475

Committed r150513: <http://trac.webkit.org/changeset/150513>
Comment 11 WebKit Commit Bot 2013-05-22 06:44:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Darin Adler 2013-05-22 11:40:46 PDT
Another way to have fixed this warning would be to have just added this line to the top of the function:

    targetFrame = 0;

We could have left the rest alone.
Comment 13 Hugo Parente Lima 2013-05-22 11:53:31 PDT
(In reply to comment #12)
> Another way to have fixed this warning would be to have just added this line to the top of the function:
> 
>     targetFrame = 0;
> 
> We could have left the rest alone.

OTOH causes a double initialization in normal cases, of a cheap pointer, but causes =]