Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update to newer ICU binaries (5.7) #668

Merged
merged 3 commits into from
Jul 22, 2016
Merged

Update to newer ICU binaries (5.7) #668

merged 3 commits into from
Jul 22, 2016

Conversation

ms-jihua
Copy link
Contributor

Fixes #667

@ms-jihua ms-jihua added this to the 1607 milestone Jul 19, 2016
bbowman
bbowman reviewed Jul 19, 2016
View reviewed changes
build/CoreFoundation/dll/CoreFoundation.vcxproj
@@ -152,7 +152,7 @@
<IgnoreAllDefaultLibraries>false</IgnoreAllDefaultLibraries>
<GenerateWindowsMetadata>false</GenerateWindowsMetadata>
<ModuleDefinitionFile>CoreFoundation.def</ModuleDefinitionFile>
<AdditionalDependencies>mincore.lib;libxml2.lib;icudt.lib;icuin.lib;icuuc.lib;libdispatch.lib;icudata.lib;%(AdditionalDependencies)</AdditionalDependencies>
Copy link
Member

@bbowman bbowman Jul 19, 2016 •
edited by ms-jihua
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icudata.lib [](start = 100, length = 11)

hooray!! Any numbers on the reduced dll size? #Resolved

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CoreFoundation.dll 18.8mb -> 0.9mb

In reply to: 71389811 [](ancestors = 71389811)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

icudt.dll 7 kb -> 25mb

In reply to: 71421042 [](ancestors = 71421042,71389811)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Net increase in size, probably because more complete data set now?

In reply to: 71421348 [](ancestors = 71421348,71421042,71389811)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that'd be the case. Also reminder to everyone that eventually we'll consume this directly from windows, meaning we'll no longer take the hit.

In reply to: 71459710 [](ancestors = 71459710,71421348,71421042,71389811)

@DHowett-MSFT
Copy link

DHowett-MSFT commented Jul 19, 2016 •
edited by ms-jihua
Loading

Does this also add the new 5.7 headers? #Resolved

@DHowett-MSFT
Copy link

(or, "should")

In reply to: 233714431 [](ancestors = 233714431)

bbowman
bbowman reviewed Jul 19, 2016
View reviewed changes
Frameworks/CoreFoundation/Locale.subproj/CFNumberFormatter.c
@@ -786,7 +786,7 @@ void CFNumberFormatterSetProperty(CFNumberFormatterRef formatter, CFStringRef ke
#if U_ICU_VERSION_MAJOR_NUM >= 55
__CFGenericValidateType(value, CFNumberGetTypeID());
CFNumberGetValue((CFNumberRef)value, kCFNumberSInt32Type, &n);
__cficu_unum_setContext(formatter->_nf, n, &status);
__cficu_unum_setContext(formatter->_nf, (UDisplayContext)n, &status);
Copy link
Member

@bbowman bbowman Jul 19, 2016 •
edited by ms-jihua
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UDisplayContext [](start = 49, length = 15)

static cast? // WINOBJC comment? #Pending

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's not static cast, this is a C file and upstreamable.

In reply to: 71390074 [](ancestors = 71390074)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// WINOBJC: WinObjC seems to have stricter type-checking than the reference platform,
// so an explicit cast is required here

In reply to: 71390930 [](ancestors = 71390930,71390074)

@bbowman
Copy link
Member

bbowman commented Jul 19, 2016 •
edited by ms-jihua
Loading

I don't see the new binaries in the commit here. What is the deal? Is that a git-lfs artifact or did they not get added accidentally? #ByDesign

@bbowman
Copy link
Member

bbowman commented Jul 19, 2016

🕐

@ms-jihua
Copy link
Contributor Author

It does

In reply to: 233714467 [](ancestors = 233714467,233714431)

@ms-jihua
Copy link
Contributor Author

I'm guessing it's because of "Sorry, we could not display the entire diff because too many files (8,133) changed."

In reply to: 233715196 [](ancestors = 233715196)

@rajsesh
Copy link
Contributor

rajsesh commented Jul 20, 2016

:shipit:

1 similar comment
@DHowett-MSFT
Copy link

:shipit:

ms-jihua added 2 commits July 22, 2016 11:53
@ms-jihua
Add support in CFCalendar for new ICU behavior
81b8a5c 
ucal_getDayOfWeekType now returns UCAL_WEEKDAY/WEEKEND instead of UCAL_WEEKEND_START, UCAL_WEEKEND_CEASE.
Added fallback cases to populate onset and cease even with the new behavior.
@ms-jihua
Removed excessive comments
d4ef6c0
@MSFTFox
Copy link
Member

MSFTFox commented Jul 22, 2016

:shipit:

1 similar comment
@msft-Jeyaram
Copy link
Contributor

:shipit:

@rajsesh rajsesh merged commit 94a244e into microsoft:develop Jul 22, 2016
@ms-jihua ms-jihua deleted the icu_0718 branch July 25, 2016 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@DHowett-MSFT DHowett-MSFT

@rajsesh rajsesh

@bbowman bbowman

@ms-jihua ms-jihua

@msft-Jeyaram msft-Jeyaram

Labels
Area-Foundation Under Review
Projects
None yet
Milestone
1607
Development

Successfully merging this pull request may close these issues.
7 participants
@ms-jihua @DHowett-MSFT @bbowman @rajsesh @MSFTFox @msft-Jeyaram @msftclas