Bug 84377 – Initialize member variables in CSSParser's constructor.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Luke Macpherson]

Initialize member variables in CSSParser's constructor.

[Luke Macpherson]

Created attachment 137965 [details]
Patch

[Alexis Menard (darktears)]

Comment on attachment 137965 [details]
Patch

INVALID_NUM_PARSED_PROPERTIES seems to be used when clearing this variable.

[Kentaro Hara]

Comment on attachment 137965 [details]
Patch

OK

[Eric Seidel (no email)]

Comment on attachment 137965 [details]
Patch

Eek.  Can this be tested?

[Kentaro Hara]

Comment on attachment 137965 [details]
Patch

Sorry for the overlooking.

[Luke Macpherson]

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]

(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]

Created attachment 137982 [details]
Patch

[Kentaro Hara]

(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]

(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]

See also the webkit-dev@ discussion: http://markmail.org/thread/i6lzp4iwwba2yua3

[Luke Macpherson]

(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]

(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]

Created attachment 138134 [details]
Patch

[Kentaro Hara]

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]

Created attachment 139407 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 139407 [details]
Patch for landing

Clearing flags on attachment: 139407

Committed r115602: <http://trac.webkit.org/changeset/115602>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.