WIP Intl numfmt #4

dilijev · 2017-09-23T07:22:47Z

No description provided.

dilijev · 2017-09-23T07:23:09Z

jackhorton

Just some lazy sunday nitpicking

jackhorton · 2017-09-24T20:06:37Z

lib/Runtime/PlatformAgnostic/Platform/CMakeLists.txt

+set(PL_SOURCE_FILES ${PL_SOURCE_FILES}
+  Common/Intl.cpp
+  )
+endif()


Not that we use CMake on Windows, but is there any other reason for OS-guarding this?

Good point.

jackhorton · 2017-09-24T20:09:47Z

lib/Runtime/PlatformAgnostic/Platform/Common/Intl.cpp

+        }
+
+    cleanup:
+        delete[] inputLangTagUtf8;


I saw that you had an auto string buffer class above, should this just be like a C++ unique_ptr generic class in our common data structures?

Yes that's basically the point, and so we don't take a dependency on the standard library for that simple logic. It might make sense to do this here as well, although the logic here was designed to ensure we clean up on failure.

jackhorton · 2017-09-24T20:13:32Z

lib/Runtime/PlatformAgnostic/Platform/Common/Intl.cpp

+    {
+        // Allocate memory for the UTF8 output buffer. Need 3 bytes for each (code point + null) to satisfy SAL.
+        const size_t inputLangTagUtf8SizeAllocated = AllocSizeMath::Mul(AllocSizeMath::Add(cch, 1), 3);
+        // REVIEW (doilij): not perf critical so I used HeapNewArrayZ to zero-out the allocated array


I remember looking at this code before and it did use HeapNewArrayZ, and while I imagine the change to new was to satisfy the operator new issues you were discussing, this comment and the following assert should probably change.

Yes, this is because we're in a compilation unit that doesn't have knowledge of the various ChakraCore allocators, hence the switch to new/delete. I should probably make the effort to explicitly zero-out these arrays. I'll update the comments.

jackhorton · 2017-09-24T20:19:21Z

lib/Runtime/PlatformAgnostic/Platform/Common/Intl.cpp

+        Output::Print(_u("EntryIntl_CurrencyDigits > returning (%d)\n"), minFracDigits);
+#endif
+
+        return minFracDigits;


The ICU documentation doesn't say anything about deleting the return value of createCurrencyInstance explicitly, but I think http://userguide.icu-project.org/dev/codingguidelines suggests that all of the create* static methods require explicit delete calls.

Yes, you're correct. Good catch.

jackhorton · 2017-09-24T20:33:45Z

lib/Runtime/PlatformAgnostic/Platform/Common/Intl.cpp

+    {
+        icu::UnicodeString result;
+
+        auto *formatterHolder = (PlatformAgnosticIntlObject<icu::NumberFormat> *)formatter;


It seems like the codebase generally favors reinterpret_cast?

Not so much that it favors (it's also hard to grep for C-style casts) but it is better practice. I'll update.

…e developing.

…ncyDigits

…th UErrorCode

…tNumber

…berFormat

…g Intl.cpp

…NumberFormat

…mber

dilijev · 2017-09-26T19:39:15Z

See chakra-core#3809

jackhorton suggested changes Sep 24, 2017

View reviewed changes

jackhorton and others added 22 commits September 25, 2017 15:35

Modify build system to only set the ICU include folder in one place

45d9e47

Update bytecode GUID. (Fixes chakra-core#3773)

988ee88

CommonDefines.h: define (commented-out) INTL_ICU for convenience whil…

19fd371

…e developing.

build: Add icui18n.lib to Windows build's link dependencies.

b4d402c

Intl.js: Intl.NumberFormat constructor: change var to let

3f42209

Closes chakra-core#3661: Provide ICU implementation of platform.curre…

ab2a327

…ncyDigits

tests: added Intl tag to Object/toLocaleStringBasics.js

fabf1bf

tests: Added require_winglob to tests which fail under Intl-ICU.

d916609

tests: Fix filename case warnings from runtests.py

e115580

cleanup: IcuIntlAdapter: be consistent about prefixing error enums wi…

10cbcaa

…th UErrorCode

Closes chakra-core#3664: Provide ICU implementation of platform.forma…

4d4cd99

…tNumber

CacheNumberFormat: Started implementation of IcuIntlAdapter::CacheNum…

533d50e

…berFormat

(checkpoint: builds) Started Refactoring to PlatformAgnostic

e9e979d

Intl.js: add 'latn' as default value for numberFormat.__numberingSystem

e5e444c

(BYTECODE)

f81bdc0

Refactored all ICU operations to PlatformAgnostic.

2b82a2c

Cleanup and accomodate IntlICU-specific build files.

64fd42e

Update CMakeLists.txt for PlatformAgnostic

495b264

Fix PlatformAgnostic CMakeLists.txt to check for IntlICU before addin…

494b0fb

…g Intl.cpp

Some xplat fixes.

847d25d

Almost fixed xplat build for --with-intl; still some missing icu symbols

5bca48f

Fix link error on linux --with-intl

679c70c

dilijev force-pushed the intl-numfmt branch 6 times, most recently from 6853614 to ff11be2 Compare September 26, 2017 00:17

dilijev force-pushed the intl-numfmt branch 8 times, most recently from 9b72065 to 5d00291 Compare September 26, 2017 07:10

Address review feedback and clean up.

fee58b8

dilijev force-pushed the intl-numfmt branch from 5d00291 to fee58b8 Compare September 26, 2017 07:13

dilijev added 2 commits September 26, 2017 04:39

Closes chakra-core#3716: Provide ICU implementation of platform.cache…

c572688

…NumberFormat

Ref chakra-core#3716: Improve ICU implementation of platform.formatNu…

94f556d

…mber

dilijev closed this Sep 26, 2017

WIP Intl numfmt #4

WIP Intl numfmt #4

Uh oh!

Conversation

dilijev commented Sep 23, 2017

Uh oh!

dilijev commented Sep 23, 2017

Uh oh!

jackhorton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dilijev commented Sep 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants