Bug 139395 – Move WebCore/platform/text to std::unique_ptr

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Gyuyoung Kim]

SSIA

[Gyuyoung Kim]

Created attachment 242808 [details]
WIP

[Gyuyoung Kim]

Created attachment 242876 [details]
Patch

[Gyuyoung Kim]

Created attachment 242978 [details]
Patch

[Gyuyoung Kim]

CC'ing Darin. Could you take a look this ?

[Mark Lam]

Comment on attachment 242978 [details]
Patch

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

> Source/WebCore/platform/text/LocaleICU.cpp:167
> +std::unique_ptr<Vector<String> > LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)

nit: please remove the space between Vector<String> and the last >.

> Source/WebCore/platform/text/LocaleICU.cpp:193
> +static std::unique_ptr<Vector<String> > createFallbackMonthLabels()

Ditto.  Remove extra space.

> Source/WebCore/platform/text/LocaleICU.cpp:217
> +static std::unique_ptr<Vector<String> > createFallbackAMPMLabels()

Ditto.  Remove extra space.

> Source/WebCore/platform/text/LocaleICU.cpp:333
> -        if (OwnPtr<Vector<String> > labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> +        auto labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {

Strange.  How did this build?  The “if (” is missing.

> Source/WebCore/platform/text/mac/LocaleMac.h:50
> +    static std::unique_ptr<LocaleMac> create(NSLocale*);

I think no one calls this now that Locale::create() constructs LocaleMac directly with a make_unique.  If no one calls this anymore, please remove it (and the impl in the cpp file as well).

[Gyuyoung Kim]

Created attachment 243182 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #5)
> Comment on attachment 242978 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242978&action=review
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:167
> > +std::unique_ptr<Vector<String> > LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)
> 
> nit: please remove the space between Vector<String> and the last >.
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:193
> > +static std::unique_ptr<Vector<String> > createFallbackMonthLabels()
> 
> Ditto.  Remove extra space.
> 
> > Source/WebCore/platform/text/LocaleICU.cpp:217
> > +static std::unique_ptr<Vector<String> > createFallbackAMPMLabels()
> 
> Ditto.  Remove extra space.

All done.

> > Source/WebCore/platform/text/LocaleICU.cpp:333
> > -        if (OwnPtr<Vector<String> > labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> > +        auto labels = createLabelVector(m_shortDateFormat, UDAT_SHORT_MONTHS, UCAL_JANUARY, 12)) {
> 
> Strange.  How did this build?  The “if (” is missing.

Weird. Anyway, fixed.

> > Source/WebCore/platform/text/mac/LocaleMac.h:50
> > +    static std::unique_ptr<LocaleMac> create(NSLocale*);
> 
> I think no one calls this now that Locale::create() constructs LocaleMac
> directly with a make_unique.  If no one calls this anymore, please remove it
> (and the impl in the cpp file as well).

Yes, If no one calls it. it should be removed.

[Mark Lam]

Comment on attachment 243182 [details]
Patch

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

Better but there’s a few other bits I missed last time.

> Source/WebCore/platform/text/LocaleICU.h:78
> +    std::unique_ptr<Vector<String> > createLabelVector(const UDateFormat*, UDateFormatSymbolType, int32_t startIndex, int32_t size);

Please remove the empty space.

> Source/WebCore/platform/text/LocaleICU.h:89
> +    std::unique_ptr<Vector<String> > m_monthLabels;

Please remove the empty space.

> Source/WebCore/platform/text/win/LocaleWin.cpp:149
>      return LocaleWin::create(LCIDFromLocale(locale));

For consistency, let’s change this to:
    return std::make_unique<LocaleWin>(LCIDFromLocale(locale));

> Source/WebCore/platform/text/win/LocaleWin.cpp:161
> -PassOwnPtr<LocaleWin> LocaleWin::create(LCID lcid)
> +std::unique_ptr<LocaleWin> LocaleWin::create(LCID lcid)
>  {
> -    return adoptPtr(new LocaleWin(lcid));
> +    return std::make_unique<LocaleWin>(lcid);
>  }

For consistency, let’s remove this.

> Source/WebCore/platform/text/win/LocaleWin.h:47
> -    static PassOwnPtr<LocaleWin> create(LCID);
> +    static std::unique_ptr<LocaleWin> create(LCID);

For consistency, let’s remove this and make the constructor public as is done in other ports.

[Gyuyoung Kim]

Created attachment 243195 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #8)
> Comment on attachment 243182 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243182&action=review
> 
> Better but there’s a few other bits I missed last time.
> 
> > Source/WebCore/platform/text/LocaleICU.h:78
> > +    std::unique_ptr<Vector<String> > createLabelVector(const UDateFormat*, UDateFormatSymbolType, int32_t startIndex, int32_t size);
> 
> Please remove the empty space.
> 
> > Source/WebCore/platform/text/LocaleICU.h:89
> > +    std::unique_ptr<Vector<String> > m_monthLabels;
> 
> Please remove the empty space.
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:149
> >      return LocaleWin::create(LCIDFromLocale(locale));
> 
> For consistency, let’s change this to:
>     return std::make_unique<LocaleWin>(LCIDFromLocale(locale));
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:161
> > -PassOwnPtr<LocaleWin> LocaleWin::create(LCID lcid)
> > +std::unique_ptr<LocaleWin> LocaleWin::create(LCID lcid)
> >  {
> > -    return adoptPtr(new LocaleWin(lcid));
> > +    return std::make_unique<LocaleWin>(lcid);
> >  }
> 
> For consistency, let’s remove this.
> 
> > Source/WebCore/platform/text/win/LocaleWin.h:47
> > -    static PassOwnPtr<LocaleWin> create(LCID);
> > +    static std::unique_ptr<LocaleWin> create(LCID);
> 
> For consistency, let’s remove this and make the constructor public as is
> done in other ports.

Ok, Fixed all.

[Mark Lam]

Comment on attachment 243195 [details]
Patch

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

> Source/WebCore/platform/text/win/LocaleWin.cpp:149
> +    return std::make_unique<LCIDFromLocale>(locale);

This is wrong.  It should be:
    return std::make_unique<LocaleWin>(LCIDFromLocale(locale));

[Gyuyoung Kim]

Created attachment 243248 [details]
Patch

[Gyuyoung Kim]

(In reply to comment #11)
> Comment on attachment 243195 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243195&action=review
> 
> > Source/WebCore/platform/text/win/LocaleWin.cpp:149
> > +    return std::make_unique<LCIDFromLocale>(locale);
> 
> This is wrong.  It should be:
>     return std::make_unique<LocaleWin>(LCIDFromLocale(locale));

Oh, my stupid mistake. Fixed.

[Darin Adler]

Comment on attachment 243248 [details]
Patch

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

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:45
> +    LineBreakIteratorPool() { } 
> +

This line of code can and should just be omitted. The only reason we needed a declaration for the empty constructor before was to make it private, since it’s public by default. Leaving this out should give us what we want.

> Source/WebCore/platform/text/LocaleICU.cpp:167
> +std::unique_ptr<Vector<String>> LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)

This function should really just return a Vector, not a unique_ptr<Vector>. But that’s something someone else can fix later, I guess.

> Source/WebCore/platform/text/LocaleICU.cpp:193
> +static std::unique_ptr<Vector<String>> createFallbackMonthLabels()

As with the above, this function should return a Vector, not a std::unique_ptr<Vector>.

> Source/WebCore/platform/text/LocaleICU.cpp:217
> +static std::unique_ptr<Vector<String>> createFallbackAMPMLabels()

As with the above, this function should return a Vector, not a std::unique_ptr<Vector>.

[Gyuyoung Kim]

Created attachment 243269 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 243248 [details]
Patch

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

>> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:45
>> +
> 
> This line of code can and should just be omitted. The only reason we needed a declaration for the empty constructor before was to make it private, since it’s public by default. Leaving this out should give us what we want.

yes, we don't need to keep empty constructor as public. Removed.

>> Source/WebCore/platform/text/LocaleICU.cpp:167
>> +std::unique_ptr<Vector<String>> LocaleICU::createLabelVector(const UDateFormat* dateFormat, UDateFormatSymbolType type, int32_t startIndex, int32_t size)
> 
> This function should really just return a Vector, not a unique_ptr<Vector>. But that’s something someone else can fix later, I guess.

Let me file a new bug for handling your comment in near future.

[Gyuyoung Kim]

Created attachment 243272 [details]
Patch

[Darin Adler]

Comment on attachment 243272 [details]
Patch

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

> Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:44
> +    LineBreakIteratorPool() { }

It’s still here, but you said you removed it. Did something go wrong?

[Gyuyoung Kim]

(In reply to comment #18)
> Comment on attachment 243272 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=243272&action=review
> 
> > Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:44
> > +    LineBreakIteratorPool() { }
> 
> It’s still here, but you said you removed it. Did something go wrong?

Because there were a build break on EFL and GTK port when I removed it as below,

WTF_MAKE_NONCOPYABLE seems that LineBreakIteratorPool class doesn't allow to make a default constructor implicitly. Thus LineBreakIteratorPool seems to need to have it explicitly.


../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:49:18:   required from here
../../Source/WTF/wtf/ThreadSpecific.h:258:9: error: no matching function for call to ‘WebCore::LineBreakIteratorPool::LineBreakIteratorPool()’
         new (NotNull, ptr) T;
         ^
../../Source/WTF/wtf/ThreadSpecific.h:258:9: note: candidate is:
In file included from ../../Source/WebCore/platform/text/TextBreakIterator.cpp:25:0:
../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:42:14: note: WebCore::LineBreakIteratorPool::LineBreakIteratorPool(const WebCore::LineBreakIteratorPool&) <deleted>
     WTF_MAKE_NONCOPYABLE(LineBreakIteratorPool);
              ^
../../Source/WebCore/platform/text/LineBreakIteratorPoolICU.h:42:14: note:   candidate expects 1 argument, 0 provided

[WebKit Commit Bot]

Comment on attachment 243272 [details]
Patch

Clearing flags on attachment: 243272

Committed r177280: <http://trac.webkit.org/changeset/177280>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.