Bug 116340 - WebCore fails to compile with -Werror=maybe-uninitialized on GCC 4.8.0
Summary: WebCore fails to compile with -Werror=maybe-uninitialized on GCC 4.8.0
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hugo Parente Lima
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-05-17 12:06 PDT by Hugo Parente Lima
Modified: 2013-05-22 11:53 PDT (History)
5 users (show)

See Also:


Attachments
Patch (2.03 KB, patch)
2013-05-17 12:10 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff
Patch (2.25 KB, patch)
2013-05-21 15:49 PDT, Hugo Parente Lima
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 =]