Bug 154484

Summary: Support building LocaleICU with light ICU (UCONFIG_NO_FORMATTING)
Product: WebKit Reporter: Olivier Blin <olivier.blin>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, commit-queue, darin, ewmailing, mcatanzaro, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Olivier Blin 2016-02-19 17:10:58 PST
LocaleICU should be buildable with a light ICU build, with the UCONFIG_NO_FORMATTING flag.

In this mode, this makes LocaleICU with UCONFIG_NO_FORMATTING essentially the same as LocaleNone, but allows to keep using ICU for other features.
Comment 1 Olivier Blin 2016-02-19 17:12:16 PST
Created attachment 271831 [details]
Patch
Comment 2 Darin Adler 2016-02-21 17:17:04 PST
Comment on attachment 271831 [details]
Patch

Patch doesn’t apply. Please upload a new patch that applies.
Comment 3 Michael Catanzaro 2016-02-21 18:48:22 PST
It depended on other patches that were not in trunk when this patch was uploaded and when EWS ran, but which are now in trunk. So it *probably* applies. Bugzilla needs some way to trigger EWS to run again; a workaround is to reupload the same patch again. I often delay marking patches r? until dependent patches have landed to avoid this problem.
Comment 4 Olivier Blin 2016-02-22 14:54:03 PST
Created attachment 271958 [details]
Patch
Comment 5 Olivier Blin 2016-02-22 14:54:52 PST
That's the same patch again, it should apply now that other patches got merged, as Michael explained.
Comment 6 Darin Adler 2016-02-23 22:29:27 PST
Comment on attachment 271958 [details]
Patch

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

> Source/WebCore/platform/text/LocaleICU.h:86
> +#if !UCONFIG_NO_FORMATTING
>      UNumberFormat* m_numberFormat;
>      bool m_didCreateDecimalFormat;
> +#endif

Would be better to initialize these data members here. Then we would not need the conditional initialization lines in the constructor.
Comment 7 WebKit Commit Bot 2016-02-23 23:20:25 PST
Comment on attachment 271958 [details]
Patch

Clearing flags on attachment: 271958

Committed r197018: <http://trac.webkit.org/changeset/197018>
Comment 8 WebKit Commit Bot 2016-02-23 23:20:29 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Olivier Blin 2016-02-24 04:15:33 PST
(In reply to comment #6)
> Comment on attachment 271958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271958&action=review
> 
> > Source/WebCore/platform/text/LocaleICU.h:86
> > +#if !UCONFIG_NO_FORMATTING
> >      UNumberFormat* m_numberFormat;
> >      bool m_didCreateDecimalFormat;
> > +#endif
> 
> Would be better to initialize these data members here. Then we would not
> need the conditional initialization lines in the constructor.

But the UNumberFormat type would be unknown, and we would have to forward-declare it to allow using a pointer on it.

Is it worth doing this?
If so, I can resend a patch.
Comment 10 Darin Adler 2016-02-24 09:40:01 PST
Comment on attachment 271958 [details]
Patch

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

>>> Source/WebCore/platform/text/LocaleICU.h:86
>>> +#endif
>> 
>> Would be better to initialize these data members here. Then we would not need the conditional initialization lines in the constructor.
> 
> But the UNumberFormat type would be unknown, and we would have to forward-declare it to allow using a pointer on it.
> 
> Is it worth doing this?
> If so, I can resend a patch.

We can initialize to null without any other changes:

#if !UCONFIG_NO_FORMATTING
    UNumberFormat* m_numberFormat { nullptr };
    bool m_didCreateDecimalFormat { false };
#endif

I don’t know what you mean by “we would have to forward-declare it”.

And yes, I do think it’s worth doing this so we can remove the four lines of code in the constructor that conditionally initialize these data members.
Comment 11 Olivier Blin 2016-02-25 08:38:06 PST
(In reply to comment #10)
> Comment on attachment 271958 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271958&action=review
> 
> >>> Source/WebCore/platform/text/LocaleICU.h:86
> >>> +#endif
> >> 
> >> Would be better to initialize these data members here. Then we would not need the conditional initialization lines in the constructor.
> > 
> > But the UNumberFormat type would be unknown, and we would have to forward-declare it to allow using a pointer on it.
> > 
> > Is it worth doing this?
> > If so, I can resend a patch.
> 
> We can initialize to null without any other changes:
> 
> #if !UCONFIG_NO_FORMATTING
>     UNumberFormat* m_numberFormat { nullptr };
>     bool m_didCreateDecimalFormat { false };
> #endif
> 
> I don’t know what you mean by “we would have to forward-declare it”.

Ok, looks good this way.
Sorry, I misinterpreted your initial comment.

> And yes, I do think it’s worth doing this so we can remove the four lines of
> code in the constructor that conditionally initialize these data members.

Then, I should do the same for the other LocalICU fields that have been made conditional in bug 154483, right?
Comment 12 Darin Adler 2016-02-25 09:14:08 PST
(In reply to comment #11)
> Then, I should do the same for the other LocalICU fields that have been made
> conditional in bug 154483, right?

It’s helpful to do this for all data members of pointer or scalar types with obvious, simple initial values such as nullptr, zero, and false, or even in some cases with slightly less obvious values in *all* classes and structs, everywhere in WebKit. Patches that do bits of that are always welcome.

And, yes, especially helpful for data members inside conditionals.
Comment 13 Olivier Blin 2016-02-29 01:46:14 PST
(In reply to comment #12)
> (In reply to comment #11)
> > Then, I should do the same for the other LocalICU fields that have been made
> > conditional in bug 154483, right?
> 
> It’s helpful to do this for all data members of pointer or scalar types with
> obvious, simple initial values such as nullptr, zero, and false, or even in
> some cases with slightly less obvious values in *all* classes and structs,
> everywhere in WebKit. Patches that do bits of that are always welcome.
> 
> And, yes, especially helpful for data members inside conditionals.

Done in bug 154731, thanks for your guidance Darin!