Use ICU for ID_START and ID_CONTINUE for Unicode 9 data

src/char-predicates.cc

Index: src/char-predicates.cc
diff --git a/src/char-predicates.cc b/src/char-predicates.cc
index dc9865b5584ff91f77269e4919842f07adf8e3b7..875d63a1b5b2e1fe9cf919c6c679668c59012736 100644
--- a/src/char-predicates.cc
+++ b/src/char-predicates.cc
@@ -2,41 +2,37 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#ifdef V8_INTL_SUPPORT

[Dan Ehrenberg, 2017/06/10 06:00:12]

Nit: When an entire file is conditionally compiled for V8_INTL_SUPPORT, the new
common practice across V8 is to do this in the build file, and then, within the
file, check that the flag is enabled and error out if not. See src/intl.cc for
an example.

[jungshik at Google, 2017/06/13 09:37:29]

Done.

 #include "src/char-predicates.h"
 
-#ifdef V8_INTL_SUPPORT
 #include "unicode/uchar.h"
 #include "unicode/urename.h"
-#endif  // V8_INTL_SUPPORT
 
 namespace v8 {
 namespace internal {
 
-bool SupplementaryPlanes::IsIDStart(uc32 c) {
-  DCHECK(c > 0xFFFF);
-#ifdef V8_INTL_SUPPORT
-  // This only works for code points in the SMPs, since ICU does not exclude
-  // code points with properties 'Pattern_Syntax' or 'Pattern_White_Space'.
-  // Code points in the SMP do not have those properties.
-  return u_isIDStart(c);
-#else
-  // This is incorrect, but if we don't have ICU, use this as fallback.
-  return false;
-#endif  // V8_INTL_SUPPORT
+bool IdentifierStart::Is(uc32 c) {
+  // cannot use u_isIDStart because it does not work for
+  // Other_ID_Start characters.
+  return u_hasBinaryProperty(c, UCHAR_ID_START) ||
+         (c < 0x60 && (c == 0x24 || c == 0x5C || c == 0x5F));
 }
 
+bool IdentifierPart::Is(uc32 c) {
+  // Can't use u_isIDPart because it does not work for
+  // Other_ID_Continue characters.
+  return u_hasBinaryProperty(c, UCHAR_ID_CONTINUE) ||
+         (c < 0x60 && (c == 0x24 || c == 0x5C || c == 0x5F)) || c == 0x200C ||
+         c == 0x200D;
+}

[Dan Ehrenberg, 2017/06/10 06:00:12]

Great, this is how the code should look!

Style nit: Did you consider using C character literals like '$'?

Just curious, was this (c < 0x60) logic based on a benchmark?

[jungshik at Google, 2017/06/13 09:37:29]

Ok. Will use them. 
No. Perhaps, my attempt to optimize is not necessary. The result is cached and
this function will not hit more than once for each character in most cases.  
What do you think? 

BTW, caching is another reason I'm not much worried about the perf. 

 
-bool SupplementaryPlanes::IsIDPart(uc32 c) {
-  DCHECK(c > 0xFFFF);
-#ifdef V8_INTL_SUPPORT
-  // This only works for code points in the SMPs, since ICU does not exclude
-  // code points with properties 'Pattern_Syntax' or 'Pattern_White_Space'.
-  // Code points in the SMP do not have those properties.
-  return u_isIDPart(c);
-#else
-  // This is incorrect, but if we don't have ICU, use this as fallback.
-  return false;
-#endif  // V8_INTL_SUPPORT
+// Ecma 262 7.0 11.2 White Space

[Dan Ehrenberg, 2017/06/10 06:00:12]

We do spec references these days in terms of anchors into the document, like

// ES#sec-white-space White Space

It'd be nice to have such a reference for IdentifierPart/IdentifierStart too.

[jungshik at Google, 2017/06/13 09:37:29]

Done.

+// gC=Zs, U+0009, U+000B, U+000C, U+FEFF

[Dan Ehrenberg, 2017/06/10 06:00:12]

I like this comment--a similar one could make IdentifierStart/IdentifierPart
easier to read too.

[jungshik at Google, 2017/06/13 09:37:29]

Done.

+bool WhiteSpace::Is(uc32 c) {
+  return (u_charType(c) == U_SPACE_SEPARATOR) ||
+         (c < 0x0D && (c == 0x09 || c == 0x0B || c == 0x0C)) || c == 0xFEFF;
 }
+
 }  // namespace internal
 }  // namespace v8
+#endif  // V8_INTL_SUPPORT