From 3534d337c6a0e1f67503f67b5f3e365b96a92074 Mon Sep 17 00:00:00 2001 From: Fredrik Roubert Date: Wed, 25 Aug 2021 18:11:20 +0200 Subject: [PATCH] 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. --- icu4c/source/common/unifiedcache.h | 63 ++++++++++++------------------ icu4c/source/i18n/datefmt.cpp | 22 +++++------ 2 files changed, 35 insertions(+), 50 deletions(-) diff --git a/icu4c/source/common/unifiedcache.h b/icu4c/source/common/unifiedcache.h index 6c88c08762a..f22481b348c 100644 --- 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 - #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 class LocaleCacheKey : public CacheKey { protected: Locale fLoc; + virtual bool equals(const CacheKeyBase &other) const { + if (!CacheKey::equals(other)) { + return false; + } + // We know this and other are of same class because equals() on + // CacheKey returned true. + return operator==(static_cast &>(other)); + } public: LocaleCacheKey(const Locale &loc) : fLoc(loc) {} LocaleCacheKey(const LocaleCacheKey &other) @@ -146,31 +155,9 @@ class LocaleCacheKey : public CacheKey { virtual int32_t hashCode() const { return (int32_t)(37u * (uint32_t)CacheKey::hashCode() + (uint32_t)fLoc.hashCode()); } - virtual bool operator == (const CacheKeyBase &other) const { - // reflexive - if (this == &other) { - return true; - } - if (!CacheKey::operator == (other)) { - return false; - } - // We know this and other are of same class because operator== on - // CacheKey returned true. - const LocaleCacheKey *fOther = - static_cast *>(&other); - return fLoc == fOther->fLoc; + inline bool operator == (const LocaleCacheKey &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 >> - inline bool operator==(const LocaleCacheKey& other) const { - return operator==(static_cast(other)); - } -#endif - virtual CacheKeyBase *clone() const { return new LocaleCacheKey(*this); } diff --git a/icu4c/source/i18n/datefmt.cpp b/icu4c/source/i18n/datefmt.cpp index 8e20e6b2eb6..46d5f110a6b 100644 --- a/icu4c/source/i18n/datefmt.cpp +++ b/icu4c/source/i18n/datefmt.cpp @@ -68,6 +68,14 @@ const DateFmtBestPattern *LocaleCacheKey::createObject( class U_I18N_API DateFmtBestPatternKey : public LocaleCacheKey { private: UnicodeString fSkeleton; +protected: + virtual bool equals(const CacheKeyBase &other) const { + if (!LocaleCacheKey::equals(other)) { + return false; + } + // We know that this and other are of same class if we get this far. + return operator==(static_cast(other)); + } public: DateFmtBestPatternKey( const Locale &loc, @@ -82,18 +90,8 @@ public: virtual int32_t hashCode() const { return (int32_t)(37u * (uint32_t)LocaleCacheKey::hashCode() + (uint32_t)fSkeleton.hashCode()); } - virtual bool operator==(const CacheKeyBase &other) const { - // reflexive - if (this == &other) { - return TRUE; - } - if (!LocaleCacheKey::operator==(other)) { - return FALSE; - } - // We know that this and other are of same class if we get this far. - const DateFmtBestPatternKey &realOther = - static_cast(other); - return (realOther.fSkeleton == fSkeleton); + inline bool operator==(const DateFmtBestPatternKey &other) const { + return fSkeleton == other.fSkeleton; } virtual CacheKeyBase *clone() const { return new DateFmtBestPatternKey(*this);