ICU-20973 Rewrite polymorphic CacheKeyBase equality operators for C++20.

The existing polymorphic equality operators that use different types for
the `this` and `other` objects are ambiguous with C++20 resolution rules
that require equality for reversed arguments.

In order to resolve that, while also possibly making the implementation
somewhat simpler overall, the implementation classes (LocaleCacheKey
and DateFmtBestPatternKey) now get normal (non-polymorphic) equality
operators that are trivially non-ambiguous (and as a bonus also don't
need any type casts), while the dynamic type checking logic is moved
into protected helper functions, which in the end are invoked
(without any ambiguity) by friend operators in the base class.

This way, all equality testing of cache key objects ends up taking one
of these two possible paths:

1. Both sides of the equality operator are of the same implementation
   type (ie. LocaleCacheKey or DateFmtBestPatternKey):

   The type specific equality operator is called directly, comparing the
   relevant attributes of the two objects directly.

2. The two sides of the equality operator are either of different types
   or of some base class type:

   The friend equality operators of CacheKeyBase call the virtual helper
   function to figure out whether the two objects are actually of the
   same type and if they are and this type is an implementation type
   then does the necessary type cast to get to 1.

diff --git a/icu4c/source/common/unifiedcache.h b/icu4c/source/common/unifiedcache.h
index 6c88c08762a45b834c4649ce429e112dd3b7b610..f22481b348cc4c04a11942fdea36d916d256bfcb 100644 (file)
--- a/icu4c/source/common/unifiedcache.h
+++ b/icu4c/source/common/unifiedcache.h
@@ -13,8 +13,6 @@
 #ifndef __UNIFIED_CACHE_H__
 #define __UNIFIED_CACHE_H__
 
-#include <type_traits>
-
 #include "utypeinfo.h"  // for 'typeid' to work
 
 #include "unicode/uobject.h"
@@ -55,11 +53,6 @@ class U_COMMON_API CacheKeyBase : public UObject {
     */
    virtual CacheKeyBase *clone() const = 0;
 
-   /**
-    * Equality operator.
-    */
-   virtual bool operator == (const CacheKeyBase &other) const = 0;
-
    /**
     * Create a new object for this key. Called by cache on cache miss.
     * createObject must add a reference to the object it returns. Note
@@ -82,12 +75,19 @@ class U_COMMON_API CacheKeyBase : public UObject {
     */
    virtual char *writeDescription(char *buffer, int32_t bufSize) const = 0;
 
-   /**
-    * Inequality operator.
-    */
-   bool operator != (const CacheKeyBase &other) const {
-       return !(*this == other);
+   friend inline bool operator==(const CacheKeyBase& lhs,
+                                 const CacheKeyBase& rhs) {
+       return lhs.equals(rhs);
    }
+
+   friend inline bool operator!=(const CacheKeyBase& lhs,
+                                 const CacheKeyBase& rhs) {
+       return !lhs.equals(rhs);
+   }
+
+ protected:
+   virtual bool equals(const CacheKeyBase& other) const = 0;
+
  private:
    mutable UErrorCode fCreationStatus;
    mutable UBool fIsPrimary;
@@ -122,11 +122,12 @@ class CacheKey : public CacheKeyBase {
        return buffer;
    }
 
+ protected:
    /**
     * Two objects are equal if they are of the same type.
     */
-   virtual bool operator == (const CacheKeyBase &other) const {
-       return typeid(*this) == typeid(other);
+   virtual bool equals(const CacheKeyBase &other) const {
+       return this == &other || typeid(*this) == typeid(other);
    }
 };
 
@@ -138,6 +139,14 @@ template<typename T>
 class LocaleCacheKey : public CacheKey<T> {
  protected:
    Locale   fLoc;
+   virtual bool equals(const CacheKeyBase &other) const {
+       if (!CacheKey<T>::equals(other)) {
+           return false;
+       }
+       // We know this and other are of same class because equals() on
+       // CacheKey returned true.
+       return operator==(static_cast<const LocaleCacheKey<T> &>(other));
+   }
  public:
    LocaleCacheKey(const Locale &loc) : fLoc(loc) {}
    LocaleCacheKey(const LocaleCacheKey<T> &other)
@@ -146,31 +155,9 @@ class LocaleCacheKey : public CacheKey<T> {
    virtual int32_t hashCode() const {
        return (int32_t)(37u * (uint32_t)CacheKey<T>::hashCode() + (uint32_t)fLoc.hashCode());
    }
-   virtual bool operator == (const CacheKeyBase &other) const {
-       // reflexive
-       if (this == &other) {
-           return true;
-       }
-       if (!CacheKey<T>::operator == (other)) {
-           return false;
-       }
-       // We know this and other are of same class because operator== on
-       // CacheKey returned true.
-       const LocaleCacheKey<T> *fOther =
-               static_cast<const LocaleCacheKey<T> *>(&other);
-       return fLoc == fOther->fLoc;
+   inline bool operator == (const LocaleCacheKey<T> &other) const {
+       return fLoc == other.fLoc;
    }
-
-#if defined(__cpp_impl_three_way_comparison) && \
-       __cpp_impl_three_way_comparison >= 201711
-    // Manually resolve C++20 reversed argument order ambiguity.
-    template <typename U,
-              typename = typename std::enable_if_t<!std::is_same_v<T, U>>>
-    inline bool operator==(const LocaleCacheKey<U>& other) const {
-        return operator==(static_cast<const CacheKeyBase&>(other));
-    }
-#endif
-
    virtual CacheKeyBase *clone() const {
        return new LocaleCacheKey<T>(*this);
    }

diff --git a/icu4c/source/i18n/datefmt.cpp b/icu4c/source/i18n/datefmt.cpp
index 8e20e6b2eb6f4fddce30b1eea2cbfdd499f455c5..46d5f110a6b10a29cc3b4b55b9b147f5bb958878 100644 (file)
--- a/icu4c/source/i18n/datefmt.cpp
+++ b/icu4c/source/i18n/datefmt.cpp
@@ -68,6 +68,14 @@ const DateFmtBestPattern *LocaleCacheKey<DateFmtBestPattern>::createObject(
 class U_I18N_API DateFmtBestPatternKey : public LocaleCacheKey<DateFmtBestPattern> { 
 private:
     UnicodeString fSkeleton;
+protected:
+    virtual bool equals(const CacheKeyBase &other) const {
+       if (!LocaleCacheKey<DateFmtBestPattern>::equals(other)) {
+           return false;
+       }
+       // We know that this and other are of same class if we get this far.
+       return operator==(static_cast<const DateFmtBestPatternKey &>(other));
+    }
 public:
     DateFmtBestPatternKey(
         const Locale &loc,
@@ -82,18 +90,8 @@ public:
     virtual int32_t hashCode() const {
         return (int32_t)(37u * (uint32_t)LocaleCacheKey<DateFmtBestPattern>::hashCode() + (uint32_t)fSkeleton.hashCode());
     }
-    virtual bool operator==(const CacheKeyBase &other) const {
-       // reflexive
-       if (this == &other) {   
-           return TRUE;
-       }
-       if (!LocaleCacheKey<DateFmtBestPattern>::operator==(other)) {
-           return FALSE;
-       }
-       // We know that this and other are of same class if we get this far.
-       const DateFmtBestPatternKey &realOther =
-               static_cast<const DateFmtBestPatternKey &>(other);
-       return (realOther.fSkeleton == fSkeleton);
+    inline bool operator==(const DateFmtBestPatternKey &other) const {
+        return fSkeleton == other.fSkeleton;
     }
     virtual CacheKeyBase *clone() const {
         return new DateFmtBestPatternKey(*this);