WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
42726
Refactor CSSSelector's destructor and make it inline.
https://bugs.webkit.org/show_bug.cgi?id=42726
Summary
Refactor CSSSelector's destructor and make it inline.
Hayato Ito
Reported
2010-07-21 00:06:48 PDT
It would be better to make CSSSelector's destructor inline. See
https://bugs.webkit.org/show_bug.cgi?id=41129#c30
for more information.
Attachments
refactor-destructor
(6.00 KB, patch)
2010-07-21 01:51 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
refactor-destructor-2
(5.92 KB, patch)
2010-07-21 09:36 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
refactor-destructor-3
(6.14 KB, patch)
2010-07-21 20:39 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
refactor-destructor-typo-fixed
(6.12 KB, patch)
2010-07-21 21:59 PDT
,
Hayato Ito
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Hayato Ito
Comment 1
2010-07-21 01:51:32 PDT
Created
attachment 62155
[details]
refactor-destructor
WebKit Review Bot
Comment 2
2010-07-21 01:54:06 PDT
Attachment 62155
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSSelector.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Hayato Ito
Comment 3
2010-07-21 02:09:18 PDT
This style issue is expected one, the result of adapting local style. (In reply to
comment #2
)
>
Attachment 62155
[details]
did not pass style-queue: > > Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 > WebCore/css/CSSSelector.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] > Total errors found: 1 in 3 files > > > If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 4
2010-07-21 08:27:31 PDT
Comment on
attachment 62155
[details]
refactor-destructor This patch is great! Thanks for taking my suggestions. I am not wholly happy with the use of the term "children" to mean the objects a selector owns. It's standard to do such a thing in a generic tree structure, but in this case neither "tag history" nor "simple selector" seems to be entirely like a child. But since I don't have better terminology to suggest, I guess you can stick with "children" unless you can think of something better.
> +inline void CSSSelector::releaseChildrenToBag(CSSSelector* selector, CSSSelectorBag* bag)
This function could, and should, take a reference to the bag instead of a pointer. I don't see why this is a static member function that takes a CSSSelector* rather than a non-static member function. It would be easier to read as a normal member function.
> + ~CSSSelector() > + { > + // We should avoid a recursive destructor call, which causes stack overflow > + // if selectors are deeply nested. > + if (m_hasRareData) { > + if (!m_data.m_rareData) > + return; > + } else if (!m_data.m_tagHistory) > + return; > + deleteChildren(); > + }
I find this comment confusing. The comment explains why we call deleteChildren instead of just calling delete directly on the objects we own, but readers of the comment probably can't figure that out. Also, the comment is above the if statement, which has a different explanation: It's optimizing the common case where the selector does not own any pointers it needs to delete. Since this is a refactoring patch, review- so you can fix the comment and the static member function.
Hayato Ito
Comment 5
2010-07-21 09:36:46 PDT
Created
attachment 62196
[details]
refactor-destructor-2
Hayato Ito
Comment 6
2010-07-21 09:40:45 PDT
I've not noticed that you've already reviewed the previous patch. Please ignore this patch. (In reply to
comment #5
)
> Created an attachment (id=62196) [details] > refactor-destructor-2
Hayato Ito
Comment 7
2010-07-21 20:39:11 PDT
Created
attachment 62256
[details]
refactor-destructor-3
Hayato Ito
Comment 8
2010-07-21 20:50:36 PDT
Hi, Darin Thank you for the review again. (In reply to
comment #4
)
> (From update of
attachment 62155
[details]
) > This patch is great! Thanks for taking my suggestions. > > I am not wholly happy with the use of the term "children" to mean the objects a selector owns. It's standard to do such a thing in a generic tree structure, but in this case neither "tag history" nor "simple selector" seems to be entirely like a child. But since I don't have better terminology to suggest, I guess you can stick with "children" unless you can think of something better.
I agree that 'children' is very confusing name. I've changed the names like: deleteChildren -> deleteReachableSelectors releaseChildrenToBag -> releaseOwnedSelectorsToBag That naming might be far better than using 'children', I guess. It is always difficult to come up with good naming for me:).
> > > +inline void CSSSelector::releaseChildrenToBag(CSSSelector* selector, CSSSelectorBag* bag) > > This function could, and should, take a reference to the bag instead of a pointer.
Done. Thank you.
> > I don't see why this is a static member function that takes a CSSSelector* rather than a non-static member function. It would be easier to read as a normal member function.
Done. This was stupid mistake. Thank you.
> > > + ~CSSSelector() > > + { > > + // We should avoid a recursive destructor call, which causes stack overflow > > + // if selectors are deeply nested. > > + if (m_hasRareData) { > > + if (!m_data.m_rareData) > > + return; > > + } else if (!m_data.m_tagHistory) > > + return; > > + deleteChildren(); > > + } > > I find this comment confusing. The comment explains why we call deleteChildren instead of just calling delete directly on the objects we own, but readers of the comment probably can't figure that out. Also, the comment is above the if statement, which has a different explanation: It's optimizing the common case where the selector does not own any pointers it needs to delete. >
Done. I updated the comments. Thank you.
> Since this is a refactoring patch, review- so you can fix the comment and the static member function.
WebKit Review Bot
Comment 9
2010-07-21 20:54:53 PDT
Attachment 62256
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSSelector.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Darin Adler
Comment 10
2010-07-21 21:35:25 PDT
Comment on
attachment 62256
[details]
refactor-destructor-3
> + // rareData must be deleted here. > + delete m_data.m_rareData;
This is not a very good comment. Good comments answer the question "why", while this just states that the code is needed without giving a reason.
> + // We can not delete the owned object(s) by simply calling delete > + // directory on them. It leads to recursive destrutor calls which
Typo here: "directory" but it should be "directly". Typo here: "destrutor" but it should be "destructor". Instead of "It leads" the sentence should say "That would lead". review+ because this patch is great except for the comment issues above, but I'll leave commit-queue on ? until you decide if you want to post another patch.
Hayato Ito
Comment 11
2010-07-21 21:59:39 PDT
Created
attachment 62264
[details]
refactor-destructor-typo-fixed
Hayato Ito
Comment 12
2010-07-21 22:02:36 PDT
Thank you for the review. I've posted another patch which only fixed the comment issues. (In reply to
comment #10
)
> (From update of
attachment 62256
[details]
) > > + // rareData must be deleted here. > > + delete m_data.m_rareData; > > This is not a very good comment. Good comments answer the question "why", while this just states that the code is needed without giving a reason.
I've removed the comment. I don't think it is important one.
> > > + // We can not delete the owned object(s) by simply calling delete > > + // directory on them. It leads to recursive destrutor calls which > > Typo here: "directory" but it should be "directly". > > Typo here: "destrutor" but it should be "destructor". > > Instead of "It leads" the sentence should say "That would lead".
Done. I am sorry for that. Thank you!
> > review+ because this patch is great except for the comment issues above, but I'll leave commit-queue on ? until you decide if you want to post another patch.
WebKit Review Bot
Comment 13
2010-07-21 22:02:44 PDT
Attachment 62264
[details]
did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/css/CSSSelector.h:33: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 14
2010-07-22 07:43:21 PDT
Comment on
attachment 62264
[details]
refactor-destructor-typo-fixed Clearing flags on attachment: 62264 Committed
r63892
: <
http://trac.webkit.org/changeset/63892
>
WebKit Commit Bot
Comment 15
2010-07-22 07:43:26 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.
Top of Page
Format For Printing
XML
Clone This Bug