Bug 118534 - [buildfix] Fix build on gcc 4.8.1 when compiling with -Werror=maybe-uninitialized
Summary: [buildfix] Fix build on gcc 4.8.1 when compiling with -Werror=maybe-uninitial...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hugo Parente Lima
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-10 10:54 PDT by Hugo Parente Lima
Modified: 2013-07-11 13:40 PDT (History)
3 users (show)

See Also:


Attachments
Patch (1.45 KB, patch)
2013-07-10 10:57 PDT, Hugo Parente Lima
ggaren: review-
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-07-10 10:54:58 PDT
The warning is triggered because an uint32_t:2 is used to hold and Enum, so when this variable is used in a switch the compiler think anything could be there.

I'm aware that Efl builds with -Werror on, but those errors make me think if is better to compile with -Werror=no-maybe-uninitialized just to avoid patches like this one that doesn't help code wise and are here just make the compiler help.
Comment 1 Hugo Parente Lima 2013-07-10 10:57:56 PDT
Created attachment 206399 [details]
Patch
Comment 2 Chris Dumez 2013-07-10 12:16:19 PDT
cc'ing JSC people.
Comment 3 Geoffrey Garen 2013-07-11 11:41:30 PDT
Comment on attachment 206399 [details]
Patch

I'd prefer to see "switch (static_cast<Enum>(info.mode))" here.

Adding the default case means the compiler won't warn us if we forget an enum value in the switch statement.
Comment 4 Hugo Parente Lima 2013-07-11 11:43:23 PDT
(In reply to comment #3)
> (From update of attachment 206399 [details])
> I'd prefer to see "switch (static_cast<Enum>(info.mode))" here.
> 
> Adding the default case means the compiler won't warn us if we forget an enum value in the switch statement.

Good point, I thought about it but avoided due to the visual ugliness :-)
Comment 5 Hugo Parente Lima 2013-07-11 11:48:48 PDT
(In reply to comment #3)
> (From update of attachment 206399 [details])
> I'd prefer to see "switch (static_cast<Enum>(info.mode))" here.
> 
> Adding the default case means the compiler won't warn us if we forget an enum value in the switch statement.

btw, the enum is annonymous.

struct ExpressionRangeInfo {
    enum {
        FatLineMode,
        FatColumnMode,
        FatLineAndColumnMode
    };
};

I'm not familiar enough with this code to suggest a name better than "FatMode" to this enum, so I would ask you a name for it.

Thanks.
Comment 6 Geoffrey Garen 2013-07-11 13:22:17 PDT
> I'm not familiar enough with this code to suggest a name better than "FatMode" to this enum, so I would ask you a name for it.

I'd suggest "Mode". This struct is simple enough that a terse name is OK.
Comment 7 Hugo Parente Lima 2013-07-11 13:40:09 PDT
(In reply to comment #6)
> > I'm not familiar enough with this code to suggest a name better than "FatMode" to this enum, so I would ask you a name for it.
> 
> I'd suggest "Mode". This struct is simple enough that a terse name is OK.

Even with the static cast g++ still blaming about:

'line' may be used uninitialized in this function [-Werror=maybe-uninitialized]

IMO better add -Werror=no-maybe-uninitialized to the build, this flag causes more headaches than advantages, until now it just generated small patches that decreased the code readability.