Bug 84377 - Initialize member variables in CSSParser's constructor.
Summary: Initialize member variables in CSSParser's constructor.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Luke Macpherson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-19 13:23 PDT by Luke Macpherson
Modified: 2012-04-29 18:27 PDT (History)
4 users (show)

See Also:


Attachments
Patch (1.31 KB, patch)
2012-04-19 13:23 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (1.31 KB, patch)
2012-04-19 14:52 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (1.93 KB, patch)
2012-04-20 11:55 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch for landing (1.97 KB, patch)
2012-04-29 17:50 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2012-04-19 13:23:02 PDT
Initialize member variables in CSSParser's constructor.
Comment 1 Luke Macpherson 2012-04-19 13:23:51 PDT
Created attachment 137965 [details]
Patch
Comment 2 Alexis Menard (darktears) 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.
Comment 3 Kentaro Hara 2012-04-19 14:11:48 PDT
Comment on attachment 137965 [details]
Patch

OK
Comment 4 Eric Seidel (no email) 2012-04-19 14:12:09 PDT
Comment on attachment 137965 [details]
Patch

Eek.  Can this be tested?
Comment 5 Kentaro Hara 2012-04-19 14:18:23 PDT
Comment on attachment 137965 [details]
Patch

Sorry for the overlooking.
Comment 6 Luke Macpherson 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.
Comment 7 Kentaro Hara 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?
Comment 8 Luke Macpherson 2012-04-19 14:52:22 PDT
Created attachment 137982 [details]
Patch
Comment 9 Kentaro Hara 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.
Comment 10 Kentaro Hara 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?
Comment 11 Kentaro Hara 2012-04-20 01:48:32 PDT
See also the webkit-dev@ discussion: http://markmail.org/thread/i6lzp4iwwba2yua3
Comment 12 Luke Macpherson 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.
Comment 13 Kentaro Hara 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@.
Comment 14 Luke Macpherson 2012-04-20 11:55:59 PDT
Created attachment 138134 [details]
Patch
Comment 15 Kentaro Hara 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.
Comment 16 Luke Macpherson 2012-04-29 17:50:13 PDT
Created attachment 139407 [details]
Patch for landing
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2012-04-29 18:27:55 PDT
All reviewed patches have been landed.  Closing bug.