Bug 51927 - Unify string table adding in AtomicString
Summary: Unify string table adding in AtomicString
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Patrick R. Gansterer
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-05 07:02 PST by Patrick R. Gansterer
Modified: 2011-01-08 19:10 PST (History)
3 users (show)

See Also:


Attachments
Patch (4.83 KB, patch)
2011-01-05 07:03 PST, Patrick R. Gansterer
darin: review+
Details | Formatted Diff | Diff
Patch (4.98 KB, patch)
2011-01-08 10:31 PST, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2011-01-05 07:02:18 PST
Unify string table adding in AtomicString
Comment 1 Patrick R. Gansterer 2011-01-05 07:03:32 PST
Created attachment 78001 [details]
Patch
Comment 2 Darin Adler 2011-01-06 14:52:45 PST
Comment on attachment 78001 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=78001&action=review

> Source/JavaScriptCore/ChangeLog:11
> +        * wtf/text/AtomicString.cpp:
> +        (WTF::addIntoStringTable):
> +        (WTF::AtomicString::add):
> +        (WTF::AtomicString::fromUTF8):

Change logs are much better when you add some per-function comments. It takes a few more minutes, but is better for the project long-term.

> Source/JavaScriptCore/wtf/text/AtomicString.cpp:78
> +static inline PassRefPtr<StringImpl> addIntoStringTable(const T& value)

I would suggest calling this “addToStringTable” rather than “addIntoStringTable”.
Comment 3 Patrick R. Gansterer 2011-01-06 14:55:36 PST
(In reply to comment #2)
> (From update of attachment 78001 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78001&action=review
> 
> > Source/JavaScriptCore/ChangeLog:11
> > +        * wtf/text/AtomicString.cpp:
> > +        (WTF::addIntoStringTable):
> > +        (WTF::AtomicString::add):
> > +        (WTF::AtomicString::fromUTF8):
> 
> Change logs are much better when you add some per-function comments. It takes a few more minutes, but is better for the project long-term.

I usually try to add per function comments, when it make sense. It seams that I need to try it harder. :-)

> 
> > Source/JavaScriptCore/wtf/text/AtomicString.cpp:78
> > +static inline PassRefPtr<StringImpl> addIntoStringTable(const T& value)
> 
> I would suggest calling this “addToStringTable” rather than “addIntoStringTable”.
I wasn't very happy with the name too.
Comment 4 Patrick R. Gansterer 2011-01-08 10:31:40 PST
Created attachment 78318 [details]
Patch
Comment 5 WebKit Commit Bot 2011-01-08 19:10:29 PST
Comment on attachment 78318 [details]
Patch

Clearing flags on attachment: 78318

Committed r75343: <http://trac.webkit.org/changeset/75343>
Comment 6 WebKit Commit Bot 2011-01-08 19:10:35 PST
All reviewed patches have been landed.  Closing bug.