Initialize member variables in CSSParser's constructor.
Created attachment 137965 [details] Patch
Comment on attachment 137965 [details] Patch INVALID_NUM_PARSED_PROPERTIES seems to be used when clearing this variable.
Comment on attachment 137965 [details] Patch OK
Comment on attachment 137965 [details] Patch Eek. Can this be tested?
Comment on attachment 137965 [details] Patch Sorry for the overlooking.
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.
(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?
Created attachment 137982 [details] Patch
(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.
(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?
See also the webkit-dev@ discussion: http://markmail.org/thread/i6lzp4iwwba2yua3
(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.
(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@.
Created attachment 138134 [details] Patch
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.
Created attachment 139407 [details] Patch for landing
Comment on attachment 139407 [details] Patch for landing Clearing flags on attachment: 139407 Committed r115602: <http://trac.webkit.org/changeset/115602>
All reviewed patches have been landed. Closing bug.