libicui18n fails to compile on Windows with MSVC 19.36.32502 due to P2468R2 #1
Comments
|
Thanks for the report. Do you know by any chance if this has been fixed in newer versions of ICU? We are overdue for an upgrade. |
|
From what I understand, it seems like they've decided to use Wno-ambiguous-reversed-operator instead, but I wasn't able to see for myself as I had other things to focus on. However, I should have some free time on Monday to check it out. Relevant discussions: https://unicode-org.atlassian.net/browse/ICU-22014 |
|
Here is a git patch that addresses the compilation issue. Note that it has not undergone extensive testing. 0001-Fix-P2468R2-with-MSVC-19.36.32502.patch From 3fbe14993f0e2c4a1c6f490b71ba56d682069060 Mon Sep 17 00:00:00 2001
From: = <[email protected]>
Date: Mon, 20 Mar 2023 11:10:38 -0400
Subject: [PATCH] Fix P2468R2 with MSVC 19.36.32502
---
icu4c/source/i18n/fmtable.cpp | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/icu4c/source/i18n/fmtable.cpp b/icu4c/source/i18n/fmtable.cpp
index dbfd3c26ba..33c0ad8505 100644
--- a/icu4c/source/i18n/fmtable.cpp
+++ b/icu4c/source/i18n/fmtable.cpp
@@ -56,7 +56,8 @@ using number::impl::DecimalQuantity;
// Return TRUE if *a == *b.
static inline UBool objectEquals(const UObject* a, const UObject* b) {
// LATER: return *a == *b;
- return *((const Measure*) a) == *((const Measure*) b);
+ return std::memcmp(static_cast<const Measure*>(a),
+ static_cast<const Measure*>(b), sizeof(Measure)) == 0;
}
// Return a clone of *a.
--
2.40.0.windows.1 |
|
No, I am 99% sure this patch is wrong (you are most likely comparing padding bytes inside |
I don't believe the compiler would insert padding for the measure object - Even if it did, the code should still be accurate since we are already comparing the entire type, aren't we? In any case, I am hesitant to rely on "waiting for upstream to release a fix," especially given their decision to use the flag approach. It could take a considerable amount of time to resolve the issue, and in the meantime, we are unable to compile certain packages because they depend on the ICU package, which happens to be a single point of failure on Windows. Should this patch cause issues as you have indicated, adding the overload to the Measure class itself would be the suitable solution - however, this could result in more significant consequences than the objectEquals method ever would |
|
@boris-kolpackov Here's the Patch that add the overload to the Measure class itself: 0001-Add-operator-for-P2468R2.patch From f37efc2c29d25ac774890e272b7accd712448631 Mon Sep 17 00:00:00 2001
From: William Roy <[email protected]>
Date: Tue, 21 Mar 2023 11:49:00 -0400
Subject: [PATCH] Add operator!= for P2468R2
---
icu4c/source/i18n/unicode/measure.h | 1 +
1 file changed, 1 insertion(+)
diff --git a/icu4c/source/i18n/unicode/measure.h b/icu4c/source/i18n/unicode/measure.h
index fa9c29351e..186751074a 100644
--- a/icu4c/source/i18n/unicode/measure.h
+++ b/icu4c/source/i18n/unicode/measure.h
@@ -88,6 +88,7 @@ class U_I18N_API Measure: public UObject {
* @stable ICU 3.0
*/
UBool operator==(const UObject& other) const;
+ UBool operator!=(const UObject& other) const;
/**
* Return a reference to the numeric value of this object. The
--
2.40.0.windows.1FYI we still need to create the definition in measure.cpp, e.g: 0001-Add-operator-definition-for-P2468R2.patch From b07978eef67ba613d83c57fca3ec12ecebf599eb Mon Sep 17 00:00:00 2001
From: William Roy <[email protected]>
Date: Tue, 21 Mar 2023 12:00:32 -0400
Subject: [PATCH] Add operator != definition for P2468R2
---
icu4c/source/i18n/measure.cpp | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/icu4c/source/i18n/measure.cpp b/icu4c/source/i18n/measure.cpp
index bffa44215e..e73f92d81b 100644
--- a/icu4c/source/i18n/measure.cpp
+++ b/icu4c/source/i18n/measure.cpp
@@ -69,6 +69,10 @@ UBool Measure::operator==(const UObject& other) const {
(unit == NULL || *unit == *m.unit);
}
+UBool Measure::operator!=(const UObject& other) const {
+ return !(this->operator==(other));
+}
+
U_NAMESPACE_END
#endif // !UCONFIG_NO_FORMATTING
--
2.40.0.windows.1 |
|
This looks better. Can you confirm that this fixes all the compilation errors in this MSVC version? |
It does, I use it currently👍 |
|
@wroyca Do you know if there is an upstream's official fix/patch for this yet? |
|
Also, 19.36.32502 is (currently) the preview version of MSVC 17.6, correct? |
Sorry late answer (timezone) but that's correct, as for upstream I didn't have a chance to look again, but you can quickly check for the overload upstream, if it's missing then the issue is still present - I'll be home a bit later today to check again |
|
I just checked 72.1 and while they didn't add the |
Thank, I'll take a look and come back to you as soon as possible 👍 |
|
@boris-kolpackov Confirmed that the issue is still present, but at a different location this time 👍 However, my patch is still applicable and resolve it: 0001-Add-operator-for-P2468R2.patch |
Not sure what this is confirming since it still seems to use |
I have tested both without my patch first, and then applied my patch, unfortunately we hit compile errors without my patch (UBool and bool alike) |
|
Ok, thanks for testing. Do you know if there is a bug report upstream for this exact problem? If not, maybe it makes sense to submit it and see what they say/plan? While your patch is sensible, because it affects a header, it will be quite messy to apply in our package. So I was hoping we could just upgrade to latest upstream and sidestep the whole mess. Another option to try would be to fix the call site in We could try explicitly calling the correct diff --git a/icu4c/source/i18n/fmtable.cpp b/icu4c/source/i18n/fmtable.cpp
index dbfd3c26..00fa750a 100644
--- a/icu4c/source/i18n/fmtable.cpp
+++ b/icu4c/source/i18n/fmtable.cpp
@@ -56,7 +56,8 @@ using number::impl::DecimalQuantity;
// Return TRUE if *a == *b.
static inline UBool objectEquals(const UObject* a, const UObject* b) {
// LATER: return *a == *b;
- return *((const Measure*) a) == *((const Measure*) b);
+ //return *((const Measure*) a) == *((const Measure*) b);
+ return ((const Measure*) a)->operator== (*((const Measure*) b));
}
// Return a clone of *a.Do you think you can try that (without your patch) and see if it helps? |
The issue with this is that anything that relies on measures, for example, will eventually encounter problems with the operator. We are fortunate that currently, only objectEquals is affected by this. That said, yes, using operator== explicitly will work! :) EDIT: Last I've check there was effectively a open issue for it but they side-stepped with Wno-ambiguous-reversed-operator on clang for now so MSVC afaik is not there yet, let me see if I can find it back EDIT: Ah yes, I've referenced it above, |
Patch upstream issue with operator== ambiguity described in GH issue #1.
|
Ok, I've published a revision with the patch to the call site. Please let me know if there are any issues. Let's also keep this bug open until a proper fix is released by upstream.
While true, this is a stop-gap measure until upstream releases a proper fix. And, as I said, it saves us from wading into a big header patching mess in our package.
None of the bug reports you've linked talk about a compile error in MSVC, so I think ICU folks are unaware of this (likely because this is a preview version of MSVC). I would suggest that you report this as a separate issue, if you can. |
The meaning of == and != expressions has been altered in C++20 with the proposals P1185R2 and P2468R2, along with changes to how overload resolution is applied to them, resulting in previously valid code becoming invalid. As a result, ICU now produces error C2666 on certain overloaded functions and operators. Due to certain packages such as Qt being dependent on ICU, it also becomes infeasible to compile them.
https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2022/p2468r2.html
The text was updated successfully, but these errors were encountered: