Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Port PR #1851 to RC1 branch#1869

Merged
joshfree merged 19 commits intodotnet:release/1.0.0-rc1from
ellismg:port-1851-to-rc
Oct 26, 2015
Merged

Port PR #1851 to RC1 branch#1869
joshfree merged 19 commits intodotnet:release/1.0.0-rc1from
ellismg:port-1851-to-rc

Conversation

@ellismg
Copy link

@ellismg ellismg commented Oct 26, 2015

This PR takes the changes from #1851 and ports them to the release branch. This change removed the need to install ICU via Homebrew on OSX.

We would like to be able to link against versions of ICU installed as a
"operating system level library" which means we can't take a dependency
on any C++ APIs.

This change moves away from icu::Calendar in favor of UCalendar. I also
introduce a small helper template to manage the lifetime of ICU
resources.
Part of the effort to remove our usage of C++ ICU APIs. The major issue
here was that the C++ API used char*'s for some things whereas the C API
used UChar*'s so we needed to define our own copies of some constants.

We also need to manage a buffer ourselves, instead of being able to use
the underlying buffer of a retured UnicodeString.
Remove uses of the C++ DateFormat and SimpleDateFormat classes in favor
of UDateFormat.

As part of this change, it was easier to move some of the code that
converts an ICU format string to a .NET Style format string from native
code up to managed code. This code used UnicodeString and we'll need to
move away from that as well as we remove all the C++ usage.
This change removes NumberFormat in favor of UNumberFormat. There is a
bit of work that needs to happen in order to keep the normalization code
we use to convert an ICU pattern so to something we can match against
working.

Instead of UnicodeStrings, the input to the normalization function is
now a UChar* and we build up a std::string during normalization.  This
allows us to also skip a conversion from UChar* back to char* so we can
find the correct pattern in our collection of patterns to examine.
Getting the regular eras is straight forward, we can do the thing we do
for other locale data and just ask ICU using a specific
UDateFormatSymbolType.  For abbreviated eras, there's no C API, but we
can try to just read the data from ICU resources and fall back to the
standard width eras if that doesn't work.
To prepare for removing icu::Locale in favor of just using the id
directly, remove all the uses of Locale methods except for .getName().
We now use GetLocale to create a Locale but then turn it into a char*
for all the helper methods.

After this change, we can update GetLocale to do locale parsing into a
char buffer and remove all the locale.getName() calls with just `locale'.
Remove all the uses of the icu::Locale type in favor of just using a
char* which is the raw locale id (which is exactly what all the ICU C
apis use for a locale).

The meat of this change si in locale.cpp to actually handle doing the
conversion from UChar* to char*.  The rest of the places are dealing
with the fallout (GetLocale now has a different signiture and the
.getName() dance is no longer needed as we have a raw locale name all
the time now).
Use "= delete" syntax to make it clear the IcuHolder copy constructor
and assignment opperators are removed.

Remove superfluous "public" modifier on the struct closers used by the
IcuHolders.
OSX ships with a copy of ICU (their globalization APIs are built on top
of it).  Since we only use stable c based APIs, we can link against it
using the methods described in using a System ICU in the ICU User's
Guide (basically we disable function renaming, don't use C++ and only
use stable APIs).

The ICU headers are not part of the SDK, so we continue to need ICU
installed via Homebrew as a build time dependency.

Fixes dotnet/corefx#3849
This matches what we do in other places in calendarData.cpp, the RAII
pattern will make it easier to not leak memory.
There were a few problems that needed to be addressed:

 - Our detection logic around testing if ICU supported a feature was
   still checking for C++ stuff instead of the coresponding C
   code (which we ended up using).

 - There was some cleanup we could do now that the OSX and other Unix
   builds were split apart
@kangaroo
Copy link

I missed this the first time around, but yay for less dependencies 👍

@ellismg
Copy link
Author

ellismg commented Oct 26, 2015

@kangaroo Thanks!

joshfree added a commit that referenced this pull request Oct 26, 2015
@joshfree joshfree merged commit af48d72 into dotnet:release/1.0.0-rc1 Oct 26, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants