Fix the implementation of phone_number_compare in accordance with the tests in PhoneNumberUtils.java.

Due to the Thailand's special case, the code became a little nastier than before, but I believe it is inevitable...

3 files changed, 67 insertions, 17 deletions

diff --git a/android/Android.mk b/android/Android.mk
index 44d77b6..41675f2 100644
--- a/android/Android.mk
+++ b/android/Android.mk
@@ -40,9 +40,13 @@ include $(BUILD_EXECUTABLE)
 #   external/sqlite/android/PhoneNumberUtilsTest.cpp
 # > ./a.out
 #
-# Note: tests related to PHONE_NUMBERS_EQUAL also exists in AndroidTests in
-# java space. Add tests if you modify this.
-
+# Note: This "test" is not recognized as a formal test. This is just for enabling developers
+#       to easily check what they modified works well or not.
+#       The formal test for phone_number_compare() is in DataBaseGeneralTest.java
+#       (as of 2009-08-02), in which phone_number_compare() is tested via sqlite's custom
+#       function "PHONE_NUMBER_COMPARE".
+#       Please add tests if you modify the implementation of PhoneNumberUtils.cpp and add
+#       test cases in PhoneNumberUtilsTest.cpp.
 include $(CLEAR_VARS)
 
 LOCAL_MODULE:= libsqlite3_phone_number_utils_test
diff --git a/android/PhoneNumberUtils.cpp b/android/PhoneNumberUtils.cpp
index d469ed7..68c0fb6 100644
--- a/android/PhoneNumberUtils.cpp
+++ b/android/PhoneNumberUtils.cpp
@@ -138,7 +138,8 @@ static bool tryGetTrunkPrefixOmittedStr(const char *str, size_t len,
  * digit to compare two phone numbers.
  */
 static int tryGetCountryCallingCode(const char *str, size_t len,
-                                    const char **new_ptr, size_t *new_len)
+                                    const char **new_ptr, size_t *new_len,
+                                    bool accept_thailand_case)
 {
     // Rough regexp:
     //  ^[^0-9*#+]*((\+|0(0|11)\d\d?|166) [^0-9*#+] $
@@ -155,8 +156,13 @@ static int tryGetCountryCallingCode(const char *str, size_t len,
             case 0:
                 if      (ch == '+') state = 1;
                 else if (ch == '0') state = 2;
-                else if (ch == '1') state = 8;
-                else if (isDialable(ch)) return -1;
+                else if (ch == '1') {
+                    if (accept_thailand_case) {
+                        state = 8;
+                    } else {
+                        return -1;
+                    }
+                } else if (isDialable(ch)) return -1;
             break;
 
             case 2:
@@ -272,8 +278,11 @@ static bool checkPrefixIsIgnorable(const char* ch, int i) {
  * In the future, we should handle trunk prefix more correctly, but as of now,
  * we just ignore it...
  */
-bool phone_number_compare(const char* a, const char* b)
+static bool phone_number_compare_inter(const char* const org_a, const char* const org_b,
+                                       bool accept_thailand_case)
 {
+    const char* a = org_a;
+    const char* b = org_b;
     size_t len_a = 0;
     size_t len_b = 0;
     if (a == NULL) {
@@ -292,9 +301,12 @@ bool phone_number_compare(const char* a, const char* b)
     size_t tmp_len_a = len_a;
     size_t tmp_len_b = len_b;
 
-    int ccc_a = tryGetCountryCallingCode(a, len_a, &tmp_a, &tmp_len_a);
-    int ccc_b = tryGetCountryCallingCode(b, len_b, &tmp_b, &tmp_len_b);
+    int ccc_a = tryGetCountryCallingCode(a, len_a, &tmp_a, &tmp_len_a, accept_thailand_case);
+    int ccc_b = tryGetCountryCallingCode(b, len_b, &tmp_b, &tmp_len_b, accept_thailand_case);
+    bool both_have_ccc = false;
     bool ok_to_ignore_prefix = true;
+    bool trunk_prefix_is_omitted_a = false;
+    bool trunk_prefix_is_omitted_b = false;
     if (ccc_a >= 0 && ccc_b >= 0) {
         if (ccc_a != ccc_b) {
             // Different Country Calling Code. Must be different phone number.
@@ -303,6 +315,7 @@ bool phone_number_compare(const char* a, const char* b)
         // When both have ccc, do not ignore trunk prefix. Without this,
         // "+81123123" becomes same as "+810123123" (+81 == Japan)
         ok_to_ignore_prefix = false;
+        both_have_ccc = true;
     } else if (ccc_a < 0 && ccc_b < 0) {
         // When both do not have ccc, do not ignore trunk prefix. Without this,
         // "123123" becomes same as "0123123"
@@ -310,9 +323,11 @@ bool phone_number_compare(const char* a, const char* b)
     } else {
         if (ccc_a < 0) {
             tryGetTrunkPrefixOmittedStr(a, len_a, &tmp_a, &tmp_len_a);
+            trunk_prefix_is_omitted_a = true;
         }
         if (ccc_b < 0) {
             tryGetTrunkPrefixOmittedStr(b, len_b, &tmp_b, &tmp_len_b);
+            trunk_prefix_is_omitted_b = true;
         }
     }
 
@@ -350,21 +365,42 @@ bool phone_number_compare(const char* a, const char* b)
     }
 
     if (ok_to_ignore_prefix) {
-        if (!checkPrefixIsIgnorable(a, i_a)) {
-            return false;
+        if ((trunk_prefix_is_omitted_a && i_a >= 0) ||
+            !checkPrefixIsIgnorable(a, i_a)) {
+            if (accept_thailand_case) {
+                // Maybe the code handling the special case for Thailand makes the
+                // result garbled, so disable the code and try again.
+                // e.g. "16610001234" must equal to "6610001234", but with
+                //      Thailand-case handling code, they become equal to each other.
+                //
+                // Note: we select simplicity rather than adding some complicated
+                //       logic here for performance(like "checking whether remaining
+                //       numbers are just 66 or not"), assuming inputs are small
+                //       enough.
+                return phone_number_compare_inter(org_a, org_b, false);
+            } else {
+                return false;
+            }
         }
-        if (!checkPrefixIsIgnorable(b, i_b)) {
-            return false;
+        if ((trunk_prefix_is_omitted_b && i_b >= 0) ||
+            !checkPrefixIsIgnorable(b, i_b)) {
+            if (accept_thailand_case) {
+                return phone_number_compare_inter(org_a, org_b, false);
+            } else {
+                return false;
+            }
         }
     } else {
         // In the US, 1-650-555-1234 must be equal to 650-555-1234,
         // while 090-1234-1234 must not be equalt to 90-1234-1234 in Japan.
         // This request exists just in US (with 1 trunk (NDD) prefix).
+        // In addition, "011 11 7005554141" must not equal to "+17005554141",
+        // while "011 1 7005554141" must equal to "+17005554141"
         //
-        // At least, in this "rough" comparison, we should ignore the prefix
-        // '1', so if the remaining non-separator number is 0, we ignore it
-        // just once.
-        bool may_be_namp = true;
+        // In this comparison, we ignore the prefix '1' just once, when
+        // - at least either does not have CCC, or
+        // - the remaining non-separator number is 1
+        bool may_be_namp = !both_have_ccc;
         while (i_a >= 0) {
             const char ch_a = a[i_a];
             if (isDialable(ch_a)) {
@@ -392,4 +428,9 @@ bool phone_number_compare(const char* a, const char* b)
     return true;
 }
 
+bool phone_number_compare(const char* a, const char* b)
+{
+    return phone_number_compare_inter(a, b, true);
+}
+
 } // namespace android
diff --git a/android/PhoneNumberUtilsTest.cpp b/android/PhoneNumberUtilsTest.cpp
index a33f10a..4d9809d 100644
--- a/android/PhoneNumberUtilsTest.cpp
+++ b/android/PhoneNumberUtilsTest.cpp
@@ -82,6 +82,7 @@ int main() {
     EXPECT_EQ("650-253-0000", "   1-650-253-0000");
     EXPECT_NE("650-253-0000", "11-650-253-0000");
     EXPECT_NE("650-253-0000", "0-650-253-0000");
+    EXPECT_EQ("555-4141", "+1-700-555-4141");
 
     EXPECT_EQ("+1 650-253-0000", "6502530000");
     EXPECT_EQ("001 650-253-0000", "6502530000");
@@ -124,6 +125,10 @@ int main() {
     // Confirm that the bug found before does not re-appear.
     EXPECT_NE("080-1234-5678", "+819012345678");
     EXPECT_EQ("650-000-3456", "16500003456");
+    EXPECT_EQ("011 1 7005554141", "+17005554141");
+    EXPECT_NE("011 11 7005554141", "+17005554141");
+    EXPECT_NE("+44 207 792 3490", "00 207 792 3490");
+    // This is not related to Thailand case. NAMP "1" + region code "661".
     EXPECT_EQ("16610001234", "6610001234");
 
     // We also need to compare two alpha addresses to make sure two different strings