chore(core): regex cleanup 🙀

- cache the icu::RegexPattern
- rename USet to avoid a conflict with the real one!

For: #9121

core/src/kmx/kmx_plus.cpp

@@ -1072,16 +1072,16 @@ COMP_KMXPLUS_USET_Helper::setUset(const COMP_KMXPLUS_USET *newUset) {
   return is_valid;
 }
 
-USet::USet(const COMP_KMXPLUS_USET_RANGE *newRange, size_t newCount)  {
+SimpleUSet::SimpleUSet(const COMP_KMXPLUS_USET_RANGE *newRange, size_t newCount)  {
   for (size_t i = 0; i < newCount; i++) {
     ranges.emplace_back(newRange[i].start, newRange[i].end);
   }
 }
 
-USet::USet() {
+SimpleUSet::SimpleUSet() {
 }
 
-bool USet::contains(km_kbp_usv ch) const {
+bool SimpleUSet::contains(km_kbp_usv ch) const {
   for (const auto &range : ranges) {
     if (range.start <= ch && range.end >= ch) {
       return true;
@@ -1091,7 +1091,7 @@ bool USet::contains(km_kbp_usv ch) const {
 }
 
 bool
-USet::valid() const {
+SimpleUSet::valid() const {
   // double check
   for (const auto &range : ranges) {
     if (!Uni_IsValid(range.start, range.end)) {
@@ -1103,7 +1103,7 @@ USet::valid() const {
 }
 
 void
-USet::dump() const {
+SimpleUSet::dump() const {
   DebugLog(" - USet size=%d", ranges.size());
   for (const auto &range : ranges) {
     if (range.start == range.end) {
@@ -1114,14 +1114,14 @@ USet::dump() const {
   }
 }
 
-USet
+SimpleUSet
 COMP_KMXPLUS_USET_Helper::getUset(KMXPLUS_USET i) const {
   if (!valid() || i >= uset->usetCount) {
     assert(false);
-    return USet(nullptr, 0); // empty set
+    return SimpleUSet(nullptr, 0); // empty set
   }
   auto &set = usets[i];
-  return USet(getRange(set.range), set.count);
+  return SimpleUSet(getRange(set.range), set.count);
 }
 
 const COMP_KMXPLUS_USET_RANGE *

core/src/kmx/kmx_plus.h

@@ -694,14 +694,16 @@ struct COMP_KMXPLUS_USET_RANGE {
 };
 
 /**
- * represents one of the uset elements
+ * represents one of the uset elements.
+ * TODO-LDML: replace this with a real icu::UnicodeSet? or at least
+ * a function producing the same?
  */
-class USet {
+class SimpleUSet {
   public:
     /** construct a set over the specified range. Data is copied. */
-    USet(const COMP_KMXPLUS_USET_RANGE* newStart, size_t newCount);
+    SimpleUSet(const COMP_KMXPLUS_USET_RANGE* newStart, size_t newCount);
     /** empty set */
-    USet();
+    SimpleUSet();
     /** true if the uset contains this char */
     bool contains(km_kbp_usv ch) const;
     /** debugging */
@@ -721,7 +723,7 @@ public:
   bool setUset(const COMP_KMXPLUS_USET *newUset);
   inline bool valid() const { return is_valid; }
 
-  USet getUset(KMXPLUS_USET list) const;
+  SimpleUSet getUset(KMXPLUS_USET list) const;
   const COMP_KMXPLUS_USET_RANGE *getRange(KMX_DWORD index) const;
 
 private:

core/src/ldml/ldml_transforms.cpp

@@ -11,20 +11,6 @@
 #include <string>
 #include "kmx/kmx_xstring.h"
 
-#if defined(HAVE_ICU4C)
-// TODO-LDML: Needed this for some compiler warnings
-#define U_FALLTHROUGH
-#include "unicode/uniset.h"
-#include "unicode/usetiter.h"
-#include "unicode/unistr.h"
-#include "unicode/regex.h"
-#include "unicode/utext.h"
-#include "unicode/utypes.h"
-#else
-#error icu4c is required for this code
-#endif
-
-
 #ifndef assert
 #define assert(x)  // TODO-LDML
 #endif
@@ -49,7 +35,7 @@ namespace ldml {
 #define DebugTran(msg, ...)
 #endif
 
-element::element(const USet &new_u, KMX_DWORD new_flags)
+element::element(const SimpleUSet &new_u, KMX_DWORD new_flags)
     : chr(), uset(new_u), flags((new_flags & ~LDML_ELEM_FLAGS_TYPE) | LDML_ELEM_FLAGS_TYPE_USET) {
 }
 
@@ -204,7 +190,7 @@ element_list::load(const kmx::kmx_plus &kplus, kmx::KMXPLUS_ELEM id) {
       km_kbp_usv ch = e.element;
       emplace_back(ch, flags); // char
     } else if (type == LDML_ELEM_FLAGS_TYPE_USET) {
-      // need to load a USet
+      // need to load a SimpleUSet
       auto u = kplus.usetHelper.getUset(e.element);
       if (!u.valid()) {
         DebugLog("Error, invalid UnicodeSet at element %d", (int)i);
@@ -406,25 +392,36 @@ reorder_group::apply(std::u32string &str) const {
   return applied;
 }
 
+transform_entry::transform_entry(const transform_entry &other) :
+  fFrom(other.fFrom), fTo(other.fTo), fFromPattern(nullptr) {
+  if (other.fFromPattern) {
+    // clone pattern
+    fFromPattern.reset(other.fFromPattern->clone());
+  }
+}
+
 transform_entry::transform_entry(const std::u32string &from, const std::u32string &to) : fFrom(from), fTo(to) {
+  assert(!fFrom.empty()); // TODO-LDML: should not happen?
+
+  if (!fFrom.empty()) {
+    const std::u16string patstr = km::kbp::kmx::u32string_to_u16string(fFrom);
+    UErrorCode status           = U_ZERO_ERROR;
+    /* const */ icu::UnicodeString patustr = icu::UnicodeString(patstr.data(), (int32_t)patstr.length());
+    // add '$' to match to end
+    patustr.append(u'$');
+    fFromPattern.reset(icu::RegexPattern::compile(patustr, 0, status));
+    assert(U_SUCCESS(status)); // TODO-LDML: may be best to propagate status up ^^
+  }
 }
 
 size_t
 transform_entry::match(const std::u32string &input) const {
-  // TODO-LDML: simple approach, new regex every time
+  assert(fFromPattern);
   // TODO-LDML: Really? can't go from u32 to UnicodeString?
-
-  const std::u16string patstr = km::kbp::kmx::u32string_to_u16string(fFrom);
-  UErrorCode status           = U_ZERO_ERROR;
-  /* const */ icu::UnicodeString patustr = icu::UnicodeString(patstr.data(), (int32_t)patstr.length());
-  // add '$' to match to end
-  patustr.append(u'$');
-  std::unique_ptr<icu::RegexPattern> pattern(icu::RegexPattern::compile(patustr, 0, status));
-  assert(U_SUCCESS(status));
-
+  UErrorCode status = U_ZERO_ERROR;
   const std::u16string matchstr = km::kbp::kmx::u32string_to_u16string(input);
   icu::UnicodeString matchustr = icu::UnicodeString(matchstr.data(), (int32_t)matchstr.length());
-  std::unique_ptr<icu::RegexMatcher> matcher(pattern->matcher(matchustr, status));
+  std::unique_ptr<icu::RegexMatcher> matcher(fFromPattern->matcher(matchustr, status));
   assert(U_SUCCESS(status));
 
   if (!matcher->find(status)) { // i.e. matches somewhere, in this case at end of str
@@ -447,58 +444,53 @@ transform_entry::match(const std::u32string &input) const {
 std::u32string
 transform_entry::apply(const std::u32string &input, size_t matchLen) const {
   // TODO-LDML: and if you thought the previous function was suboptimal,
-  // TODO-LDML: now we're going to do it all again!
-  // TODO-LDML: simple approach, new regex every time
+  // TODO-LDML: We should cache the RegexMatcher from the previous call.
   // TODO-LDML: Really? can't go from u32 to UnicodeString?
 
-  const std::u16string patstr = km::kbp::kmx::u32string_to_u16string(fFrom);
-  UErrorCode status           = U_ZERO_ERROR;
-  /* const */ icu::UnicodeString patustr = icu::UnicodeString(patstr.data(), (int32_t)patstr.length());
-  // add '$' to match to end
-  patustr.append(u'$');
-  std::unique_ptr<icu::RegexPattern> pattern(icu::RegexPattern::compile(patustr, 0, status));
-  assert(U_SUCCESS(status));
+  assert(fFromPattern);
+  // TODO-LDML: simple approach, new regex every time
+  // TODO-LDML: Really? can't go from u32 to UnicodeString?
+  UErrorCode status = U_ZERO_ERROR;
 
   // we know the matchLen so we can slice the string…
-  const std::u16string matchstr = km::kbp::kmx::u32string_to_u16string(input.substr(input.length()-matchLen, matchLen));
-  icu::UnicodeString matchustr = icu::UnicodeString(matchstr.data(), (int32_t)matchstr.length());
-  std::unique_ptr<icu::RegexMatcher> matcher(pattern->matcher(matchustr, status));
+  // Note: the matchstr here (unlike in transform_entry::match) is sliced so that
+  // it only includes the matched portion. This way, when changed it's suitable as the
+  // output string.
+  const std::u16string matchstr = km::kbp::kmx::u32string_to_u16string(input.substr(input.length() - matchLen, matchLen));
+  icu::UnicodeString matchustr  = icu::UnicodeString(matchstr.data(), (int32_t)matchstr.length());
+  std::unique_ptr<icu::RegexMatcher> matcher(fFromPattern->matcher(matchustr, status));
   assert(U_SUCCESS(status));
-  // assert(matcher->find(status)); // it better match
+  // now, do the replace
   const std::u16string rstr = km::kbp::kmx::u32string_to_u16string(fTo);
-  icu::UnicodeString rustr = icu::UnicodeString(rstr.data(), (int32_t)rstr.length());
+  icu::UnicodeString rustr  = icu::UnicodeString(rstr.data(), (int32_t)rstr.length());
+  // This replace will apply $1, $2 etc.  TODO-LDML it will NOT handle mapFrom or mapTo.
   icu::UnicodeString output = matcher->replaceFirst(rustr, status);
   assert(U_SUCCESS(status));
 
-  // if (!matcher->find(status)) { // i.e. matches somewhere, in this case at end of str
-  //   return 0; // and tear everything down
-  // }
+  if (output.length() == 0) {
+    return std::u32string(); // special case of a zero length output (such as delete)
+  }
 
-
-
-  // // TODO-LDML: this is UTF-16 len, not UTF-32 len!!
-  // // auto matchLen = matcher->end64(status) - matcher->start64(status);
-  // // TODO-LDML: if we had an underlying UText this would be simpler.
-  // auto matchStart = matcher->start64(status);
-  // auto matchEnd   = matcher->end64(status);
-  // // extract..
-  // const icu::UnicodeString substr = matchustr.tempSubStringBetween((int32_t)matchStart, (int32_t)matchEnd);
-  // // preflight to UTF-32 to get length
-  // auto matchLen = substr.toUTF32(nullptr, 0, status);
-
-  // return matchLen;
-  assert(U_SUCCESS(status));
+  // TODO-LDML: All we are trying to do is to extract the output string. Probably too many steps.
   UErrorCode preflightStatus = U_ZERO_ERROR;
-  auto out32len = output.toUTF32(nullptr, 0, preflightStatus);
-  char32_t *s = new char32_t[out32len+1];
-  output.toUTF32((UChar32*)s, out32len+1, status);
-  std::u32string out32(s, out32len);
+  // calculate how big the buffer is
+  auto out32len              = output.toUTF32(nullptr, 0, preflightStatus); // preflightStatus will be an err, because we know the buffer overruns zero bytes
+  // allocate
+  char32_t *s                = new char32_t[out32len + 1];
+  assert(s != nullptr);
+  // convert
+  output.toUTF32((UChar32 *)s, out32len + 1, status);
   assert(U_SUCCESS(status));
+  // now, build a u32string
+  std::u32string out32(s, out32len);
+  // clean up buffer
+  delete [] s;
   return out32;
 }
 
 any_group::any_group(const transform_group &g) : type(any_group_type::transform), transform(g), reorder() {
 }
+
 any_group::any_group(const reorder_group &g) : type(any_group_type::reorder), transform(), reorder(g) {
 }
 

core/src/ldml/ldml_transforms.hpp

@@ -14,11 +14,23 @@
 #include <unordered_map>
 #include <utility>
 
+#if !defined(HAVE_ICU4C)
+#error icu4c is required for this code
+#endif
+
+#define U_FALLTHROUGH
+#include "unicode/utypes.h"
+#include "unicode/uniset.h"
+#include "unicode/usetiter.h"
+#include "unicode/unistr.h"
+#include "unicode/regex.h"
+#include "unicode/utext.h"
+
 namespace km {
 namespace kbp {
 namespace ldml {
 
-using km::kbp::kmx::USet;
+using km::kbp::kmx::SimpleUSet;
 
 /**
  * Type of a group
@@ -33,12 +45,11 @@ enum any_group_type {
  */
 class element {
 public:
-  /** construct from a USet */
-  element(const USet &u, KMX_DWORD flags);
+  /** construct from a SimpleUSet */
+  element(const SimpleUSet &u, KMX_DWORD flags);
   /** construct from a single char */
   element(km_kbp_usv ch, KMX_DWORD flags);
-
-  /** @returns true if a USet type */
+  /** @returns true if a SimpleUSet type */
   bool is_uset() const;
   /** @returns true if prebase bit set*/
   bool is_prebase() const;
@@ -58,7 +69,7 @@ public:
 private:
   // TODO-LDML: support multi-char strings?
   const km_kbp_usv chr;
-  const USet uset;
+  const SimpleUSet uset;
   const KMX_DWORD flags;
 };
 
@@ -67,6 +78,7 @@ private:
  */
 class transform_entry {
 public:
+  transform_entry(const transform_entry &other);
   transform_entry(
       const std::u32string &from,
       const std::u32string &to
@@ -84,8 +96,9 @@ public:
   std::u32string apply(const std::u32string &input, size_t matchLen) const;
 
 private:
-  const std::u32string fFrom;  // TODO-LDML: regex
+  const std::u32string fFrom;
   const std::u32string fTo;
+  std::unique_ptr<icu::RegexPattern> fFromPattern;
 };
 
 /**

core/tests/unit/ldml/ldml_test_source.cpp

@@ -35,6 +35,7 @@
 #if defined(HAVE_ICU4C)
 // TODO-LDML: Needed this for some compiler warnings
 #define U_FALLTHROUGH
+#include "unicode/utypes.h"
 #include "unicode/uniset.h"
 #include "unicode/usetiter.h"
 #else

core/tests/unit/ldml/test_kmx_plus.cpp

@@ -152,12 +152,12 @@ int test_uset() {
     {0x127, 0x127} // [ħ]
   };
 
-  USet u0(&r[0], 2);
+  SimpleUSet u0(&r[0], 2);
   assert_equal(u0.contains(0x62), true); // b
   assert_equal(u0.contains(0x41), false); // A
   assert_equal(u0.contains(0x127), true); // ħ
 
-  USet uempty;
+  SimpleUSet uempty;
   assert_equal(uempty.contains(0x62), false);
   assert_equal(uempty.contains(0x127), false);
 

core/tests/unit/ldml/test_transforms.cpp

@@ -166,7 +166,7 @@ test_reorder_standalone() {
                                               COMP_KMXPLUS_USET_RANGE(0x1A75, 0x1A79)};
     const COMP_KMXPLUS_USET_USET usets[]        = {{0, 1, 0xFFFFFFFF}};
     const COMP_KMXPLUS_USET_USET &toneMarksUset = usets[0];
-    const USet toneMarks(&ranges[toneMarksUset.range], toneMarksUset.count);
+    const SimpleUSet toneMarks(&ranges[toneMarksUset.range], toneMarksUset.count);
     // validate that the range [1A75, 1A79] matches
     assert_equal(toneMarks.contains(0x1A76), true);
     assert_equal(toneMarks.contains(0x1A60), false);