Bug 84377

Summary: Initialize member variables in CSSParser's constructor.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: CSSAssignee: Luke Macpherson <macpherson>
Status: RESOLVED FIXED    
Severity: Normal CC: haraken, macpherson, menard, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing none

Luke Macpherson
Reported 2012-04-19 13:23:02 PDT
Initialize member variables in CSSParser's constructor.
Attachments
Patch (1.31 KB, patch)
2012-04-19 13:23 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (1.31 KB, patch)
2012-04-19 14:52 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch (1.93 KB, patch)
2012-04-20 11:55 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Patch for landing (1.97 KB, patch)
2012-04-29 17:50 PDT, Luke Macpherson 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Luke Macpherson
Comment 1 2012-04-19 13:23:51 PDT
Created attachment 137965 [details] Patch
Alexis Menard (darktears)
Comment 2 2012-04-19 14:05:09 PDT
Comment on attachment 137965 [details] Patch INVALID_NUM_PARSED_PROPERTIES seems to be used when clearing this variable.
Kentaro Hara
Comment 3 2012-04-19 14:11:48 PDT
Comment on attachment 137965 [details] Patch OK
Eric Seidel (no email)
Comment 4 2012-04-19 14:12:09 PDT
Comment on attachment 137965 [details] Patch Eek. Can this be tested?
Kentaro Hara
Comment 5 2012-04-19 14:18:23 PDT
Comment on attachment 137965 [details] Patch Sorry for the overlooking.
Luke Macpherson
Comment 6 2012-04-19 14:25:59 PDT
I don't have any particular bug report for this - it's one that has been turned up by static analysis of the code. So in some sense this is tested, in that we regularly run static analysis that detects uninitialized member variables.
Kentaro Hara
Comment 7 2012-04-19 14:37:15 PDT
(In reply to comment #6) > I don't have any particular bug report for this - it's one that has been turned up by static analysis of the code. So in some sense this is tested, in that we regularly run static analysis that detects uninitialized member variables. - We want a test case for each change as long as we can test it. - However, in this case, testing the change is difficult. This is because the initialization value does not matter. In other words, even an uninitialized value would not have caused any issue. - As darktears@ pointed out, if we want to initialize it (we should), it would make sense to initialize it with (not 0 but) INVALID_NUM_PARSED_PROPERTIES. Right?
Luke Macpherson
Comment 8 2012-04-19 14:52:22 PDT
Created attachment 137982 [details] Patch
Kentaro Hara
Comment 9 2012-04-19 15:16:53 PDT
(In reply to comment #7) > (In reply to comment #6) > > I don't have any particular bug report for this - it's one that has been turned up by static analysis of the code. So in some sense this is tested, in that we regularly run static analysis that detects uninitialized member variables. > > - We want a test case for each change as long as we can test it. > - However, in this case, testing the change is difficult. This is because the initialization value does not matter. In other words, even an uninitialized value would not have caused any issue. > - As darktears@ pointed out, if we want to initialize it (we should), it would make sense to initialize it with (not 0 but) INVALID_NUM_PARSED_PROPERTIES. > > Right? I want to confirm the above understanding before r+ it.
Kentaro Hara
Comment 10 2012-04-19 16:39:18 PDT
(In reply to comment #9) > > - We want a test case for each change as long as we can test it. > > - However, in this case, testing the change is difficult. This is because the initialization value does not matter. In other words, even an uninitialized value would not have caused any issue. > > - As darktears@ pointed out, if we want to initialize it (we should), it would make sense to initialize it with (not 0 but) INVALID_NUM_PARSED_PROPERTIES. > > > > Right? > > I want to confirm the above understanding before r+ it. Luke: Is my understanding right?
Kentaro Hara
Comment 11 2012-04-20 01:48:32 PDT
See also the webkit-dev@ discussion: http://markmail.org/thread/i6lzp4iwwba2yua3
Luke Macpherson
Comment 12 2012-04-20 10:59:47 PDT
(In reply to comment #10) > (In reply to comment #9) > > > - We want a test case for each change as long as we can test it. > > > - However, in this case, testing the change is difficult. This is because the initialization value does not matter. In other words, even an uninitialized value would not have caused any issue. > > > - As darktears@ pointed out, if we want to initialize it (we should), it would make sense to initialize it with (not 0 but) INVALID_NUM_PARSED_PROPERTIES. > > > > > > Right? > > > > I want to confirm the above understanding before r+ it. > > Luke: Is my understanding right? Yes.
Kentaro Hara
Comment 13 2012-04-20 11:02:33 PDT
(In reply to comment #12) > > Luke: Is my understanding right? > > Yes. OK, then please write the rational to ChangeLog, instead of "No new tests / code cleanup." And let me wait for r+ it until we reach a consensus on how we should treat such kind of fixes in webkit-dev@.
Luke Macpherson
Comment 14 2012-04-20 11:55:59 PDT
Created attachment 138134 [details] Patch
Kentaro Hara
Comment 15 2012-04-23 11:14:50 PDT
Comment on attachment 138134 [details] Patch It seems we reached consensus on this patch. Please explain that "This bug is detected by static analyzer ..." in ChangeLog, as maciej pointed out.
Luke Macpherson
Comment 16 2012-04-29 17:50:13 PDT
Created attachment 139407 [details] Patch for landing
WebKit Review Bot
Comment 17 2012-04-29 18:27:51 PDT
Comment on attachment 139407 [details] Patch for landing Clearing flags on attachment: 139407 Committed r115602: <http://trac.webkit.org/changeset/115602>
WebKit Review Bot
Comment 18 2012-04-29 18:27:55 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.