Bug 154484 – Support building LocaleICU with light ICU (UCONFIG_NO_FORMATTING)

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Olivier Blin]

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.

[Olivier Blin]

Created attachment 271831 [details]
Patch

[Darin Adler]

Comment on attachment 271831 [details]
Patch

Patch doesn’t apply. Please upload a new patch that applies.

[Michael Catanzaro]

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.

[Olivier Blin]

Created attachment 271958 [details]
Patch

[Olivier Blin]

That's the same patch again, it should apply now that other patches got merged, as Michael explained.

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 271958 [details]
Patch

Clearing flags on attachment: 271958

Committed r197018: <http://trac.webkit.org/changeset/197018>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Olivier Blin]

(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.

[Darin Adler]

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.

[Olivier Blin]

(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?

[Darin Adler]

(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.

[Olivier Blin]

(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!