Summary: | Support building LocaleICU with light ICU (UCONFIG_NO_FORMATTING) | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Olivier Blin <olivier.blin> | ||||||
Component: | Platform | Assignee: | 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
Olivier Blin
2016-02-19 17:10:58 PST
Created attachment 271831 [details]
Patch
Comment on attachment 271831 [details]
Patch
Patch doesn’t apply. Please upload a new patch that applies.
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. Created attachment 271958 [details]
Patch
That's the same patch again, it should apply now that other patches got merged, as Michael explained. 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 on attachment 271958 [details] Patch Clearing flags on attachment: 271958 Committed r197018: <http://trac.webkit.org/changeset/197018> All reviewed patches have been landed. Closing bug. (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 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. (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? (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. (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! |