[RFC] ext/intl: Implement IntlRelativeDateTimeFormatter

[LamentXU123]

Implement: https://wiki.php.net/rfc/reldateformatter
Discussion: https://externals.io/message/132187

TODO:

• Complete the RFC process
• Better tests coverage
• NEWS / UPGRADING after 8.6 branch out
• A full and closer review by me weeks after this is written

After these, this PR is ready for reviews. However, I am glad to see any ideas code-wise anytime if possible :)

[LamentXU123]

Okay this is now fine for reviews. Given the current voting status this should be accepted. https://wiki.php.net/rfc/reldateformatter

[devnexen]

--- a/ext/intl/reldateformatter/reldateformatter_class.cpp
+++ b/ext/intl/reldateformatter/reldateformatter_class.cpp
@@
 struct reldateformatter_data {
+       Locale locale;
        std::unique_ptr<icu::RelativeDateTimeFormatter> formatter;
        std::unique_ptr<icu::BreakIterator> sentence_iterator;
 };
@@ static reldateformatter_data *reldateformatter_open(
-       Locale icu_locale(locale);
+       result->locale = Locale(locale);
        if (capitalization_context == UDISPCTX_CAPITALIZATION_FOR_BEGINNING_OF_SENTENCE) {
                /* ICU can fail to create this iterator before adopting the number formatter. */
-               result->sentence_iterator.reset(icu::BreakIterator::createSentenceInstance(icu_locale, *status));
+               result->sentence_iterator.reset(icu::BreakIterator::createSentenceInstance(result->locale, *status));
@@
        result->formatter.reset(new icu::RelativeDateTimeFormatter(
-               icu_locale, number_formatter_clone.release(), style, capitalization_context, *status));
+               result->locale, number_formatter_clone.release(), style, capitalization_context, *status));
@@ static zend_string *reldateformatter_format_result(
-               result.toTitle(sentence_iterator, Locale(ZSTR_VAL(obj->locale)),
+               result.toTitle(sentence_iterator, RELDATEFORMATTER_OBJECT(obj)->locale,
                        U_TITLECASE_NO_LOWERCASE | U_TITLECASE_NO_BREAK_ADJUSTMENT);

[devnexen]

apart of this looks ok even tough C++11 builds might worry me a little bit (maybe for nothing).

[LamentXU123]

Good point, Thank you!

[LamentXU123]

C++11 builds might worry me a little bit

Well Actually only starting from C++17 we have define the order of release().

Ah, I think we should instead pass get() when contructing and after the return value of new is not empty we release. And check for ICU status code.

[LamentXU123]

i.e.

--- a/ext/intl/reldateformatter/reldateformatter_class.cpp
+++ b/ext/intl/reldateformatter/reldateformatter_class.cpp
@@ -241,18 +241,20 @@ static reldateformatter_data *reldateformatter_open(
 		}
 	}
 
-	/* If allocation fails, the constructor arguments (including release()) are not evaluated.
-	 * With a valid style/context and no sentence iterator to create, ICU adopts the clone
-	 * before any initialization failure. */
+	/* C++11 may evaluate constructor arguments even if allocation fails, so keep
+	 * ownership of the clone until we know that the constructor ran. */
 	result->formatter.reset(new icu::RelativeDateTimeFormatter(
-		result->locale, number_formatter_clone.release(), style, capitalization_context, *status));
-	if (U_FAILURE(*status)) {
-		return nullptr;
-	}
+		result->locale, number_formatter_clone.get(), style, capitalization_context, *status));
 	if (result->formatter == nullptr) {
 		*status = U_MEMORY_ALLOCATION_ERROR;
 		return nullptr;
 	}
+	/* With a valid style/context and no sentence iterator to create, the constructor
+	 * adopts the clone even if initialization fails. Release it before checking status. */
+	number_formatter_clone.release();
+	if (U_FAILURE(*status)) {
+		return nullptr;
+	}
 
 	return result.release();
 }

[devnexen]

do you still need it ?

[devnexen]

You might not need this, see.