Bug 1874364
| Summary: | rapidjson does not compile with clang++ and C++20 | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Avi Kivity <avi.kivity> |
| Component: | rapidjson | Assignee: | Tom Hughes <tom> |
| Status: | CLOSED ERRATA | QA Contact: | Fedora Extras Quality Assurance <extras-qa> |
| Severity: | high | Docs Contact: | |
| Priority: | unspecified | ||
| Version: | 32 | CC: | besser82, tom, trpost |
| Target Milestone: | --- | ||
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | rapidjson-1.1.0-13.fc32 | Doc Type: | If docs needed, set a value |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2020-10-13 20:34:55 UTC | Type: | Bug |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
Please don't - when upstream ships a new version we will build it for Fedora. There's no need to patch it in Fedora. If you really want to do something useful I would suggest persuading upstream to ship a new release - they haven't released for four years despite having many, many changes since then in the repository. It's not going to be feasible to start patching something that is four years out of date as there will likely be all sorts of knock on effects and it's just not a reasonable thing to do. I don't think I have the power to persuade them to release, when they haven't for four years. It's customary in Fedora to patch upstream packages like Boost (please see changelogs) rather than keeping users with no alternative other than building unpackaged versions. This github issue[1] is full of people asking for a release. I added myself but I don't have much hope. [1] https://github.com/Tencent/rapidjson/issues/1006 In fact there are many such issues: #1006, #1130, #1265, #1282, #1341, #1483, #1546, #1550, #1585, #1612, #1759. I'm not sure Jonathan patching boost (and I have no idea how often he does that) is very comparable as he's much closer to upstream and to the language to understand the ramifications of what he's doing. I'm also pretty sure he doesn't generally do that it released versions of Fedora. I'm not saying we can't do it - your proposed patch is smaller than I was fearing. I'm reluctant to do it in released versions though and I do really want to better understand what the problem is and whether that is a minimal patch because I'm not sure it is. I'm not sure to what extent you have investigated the underlying issues, or whether you'd just copy and pasted from upstream? I can investigate myself but probably not immediately. Right. So I've looked at your actual error now, which is indeed the same as the upstream reported error, but I believe it's actually a compiler bug. Your code has a != comparison between two objects which implement the full set of six comparison operators but the compiler is telling us that it's ambiguous between operator!=() and operator==() which seems a bit odd - why is it considering operator==() you might ask? Well the answer is that part of the new spaceship operator in C++20 allows rewriting of != as == instead, as described in https://devblogs.microsoft.com/cppblog/simplify-your-code-with-rocket-science-c20s-spaceship-operator/#looks-like-a-duck-swims-like-a-duck-and-quacks-like-operator. However when that happens the rewritten code using operator==() is supposed to be lower priority in the overload resolution set to the user written operator!=() as described in https://devblogs.microsoft.com/cppblog/simplify-your-code-with-rocket-science-c20s-spaceship-operator/#old-code-wont-break which is helpfully headed "Old Code Won’t Break". The problem seems to be that clang is getting that wrong, probably due to an incomplete C++20 implementation, and is indeed breaking old code by considering the two options to be ambiguous. The patch does "fix" that by only defining the spaceship operator when the compiler supports the spaceship operator. That's certainly a good future enhancement but it shouldn't actually be necessary for correctness by my understanding of what is happening. So here's a minimal test case:
template<bool Const>
struct IntWrapper {
int value;
constexpr IntWrapper(int value) : value{value} { }
IntWrapper(const IntWrapper<false> &from) : value(from.value) {}
bool operator==(IntWrapper<true> rhs) const { return value == rhs.value; }
bool operator!=(IntWrapper<true> rhs) const { return value != rhs.value; }
};
bool foo(void)
{
IntWrapper<false> x{ 1 };
IntWrapper<false> y{ 2 };
return x != y;
}
That compiles fine with "g++ -std=c++20" but errors in the same way as you are seeing with "clang++ -std=c++20" but will work if you comment out the operator==() function.
Interesting clang errors when != is disabled (thus forcing use of the rewritten version) while gcc does not. It also give quite a specific error:
op.cpp:15:12: warning: ISO C++20 considers use of overloaded operator '!=' (with operand types 'IntWrapper<false>' and 'IntWrapper<false>') to be ambiguous despite there being a unique best viable function [-Wambiguous-reversed-operator]
return x != y;
~ ^ ~
op.cpp:6:8: note: ambiguity is between a regular call to this operator and a call with the argument order reversed
bool operator==(IntWrapper<true> rhs) const { return value == rhs.value; }
^
1 warning generated.
Which is kind of suggesting that this very deliberate so maybe it's gcc that is not up to the full spec yet? Certainly it's true that you need the templates (and resulting conversions) involved before this errors.
After consulting C++ experts it seems than clang is correct and it's actually a bug that gcc is not complaining - the conversions mean that the != call also has lower priority, equal to the priority of the rewritten version using == apparently. I have applied a minimal version of the upstream changes that I hope fixes the issue without adding extra features like the spaceship operator. Builds for F33 and rawhide available. If you can confirm that those fix your problem then we can consider an F32 build but we'll probably have to cherry pick this change as F33 already has other changes that probably shouldn't be made in a released version. Sorry for the huge delay (it turned out I had approximately 32,109 other issues with clang). I can confirm now the fix in Fedora 33 is good, and will appreciate a backport to Fedora 32. FEDORA-2020-3307ecc210 has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-3307ecc210 FEDORA-2020-3307ecc210 has been pushed to the Fedora 32 testing repository. In short time you'll be able to install the update with the following command: `sudo dnf upgrade --enablerepo=updates-testing --advisory=FEDORA-2020-3307ecc210` You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2020-3307ecc210 See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates. I confirm the Fedora 32 update works. Thanks. FEDORA-2020-3307ecc210 has been pushed to the Fedora 32 stable repository. If problem still persists, please make note of it in this bug report. |
Description of problem: Rapidjson does not compile. Version-Release number of selected component (if applicable): rapidjson-1.1.0-12.fc32.x86_64 How reproducible: Always Steps to Reproduce: 1. Try to compare two MemberIterators, for example a small for loop, with clang++ and C++20 Actual results: error: use of overloaded operator '!=' is ambiguous (with operand types 'rapidjson::GenericMemberIterator<false, rapidjson::UTF8<char>, rapidjson::CrtAllocator>' and 'rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::CrtAllocator>::MemberIterator' (aka 'rapidjson::GenericMemberIterator<false, rapidjson::UTF8<char>, rapidjson::CrtAllocator>')) for (auto it = root.MemberBegin(); it != root.MemberEnd(); ++it) { ~~ ^ ~~~~~~~~~~~~~~~~ /usr/include/rapidjson/document.h:172:10: note: candidate function bool operator!=(ConstIterator that) const { return ptr_ != that.ptr_; } ^ /usr/include/rapidjson/document.h:171:10: note: candidate function bool operator==(ConstIterator that) const { return ptr_ == that.ptr_; } ^ /usr/include/rapidjson/document.h:171:10: note: candidate function (with reversed parameter order) /usr/include/rapidjson/document.h:732:58: error: use of overloaded operator '!=' is ambiguous (with operand types 'rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::CrtAllocator>::MemberIterator' (aka 'rapidjson::GenericMemberIterator<false, rapidjson::UTF8<char>, rapidjson::CrtAllocator>') and 'rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::CrtAllocator>::MemberIterator') for (MemberIterator m = MemberBegin(); m != MemberEnd(); ++m) ~ ^ ~~~~~~~~~~~ ./utils/rjson.hh:100:12: note: in instantiation of member function 'rapidjson::GenericValue<rapidjson::UTF8<char>, rapidjson::CrtAllocator>::~GenericValue' requested here return rjson::value(rapidjson::kNullType); ^ /usr/include/rapidjson/document.h:172:10: note: candidate function bool operator!=(ConstIterator that) const { return ptr_ != that.ptr_; } ^ /usr/include/rapidjson/document.h:171:10: note: candidate function bool operator==(ConstIterator that) const { return ptr_ == that.ptr_; } ^ /usr/include/rapidjson/document.h:171:10: note: candidate function (with reversed parameter order) Expected results: Builds quietly. Additional info: Patches are already availble in rapidjson.git. I will submit a patch for the rpm.