Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 1874364 - rapidjson does not compile with clang++ and C++20
Summary: rapidjson does not compile with clang++ and C++20
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: rapidjson
Version: 32
Hardware: All
OS: Linux
unspecified
high
Target Milestone: ---
Assignee: Tom Hughes
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2020-09-01 07:51 UTC by Avi Kivity
Modified: 2020-10-13 20:34 UTC (History)
3 users (show)

Fixed In Version: rapidjson-1.1.0-13.fc32
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2020-10-13 20:34:55 UTC
Type: Bug
Embargoed:


Attachments (Terms of Use)

Description Avi Kivity 2020-09-01 07:51:58 UTC
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.

Comment 1 Tom Hughes 2020-09-01 07:54:57 UTC
Please don't - when upstream ships a new version we will build it for Fedora. There's no need to patch it in Fedora.

Comment 2 Tom Hughes 2020-09-01 07:57:47 UTC
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.

Comment 3 Avi Kivity 2020-09-01 13:53:55 UTC
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.

Comment 4 Avi Kivity 2020-09-01 13:56:04 UTC
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

Comment 5 Avi Kivity 2020-09-01 13:59:00 UTC
In fact there are many such issues: #1006, #1130, #1265, #1282, #1341, #1483, #1546, #1550, #1585, #1612, #1759.

Comment 6 Tom Hughes 2020-09-01 14:43:04 UTC
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.

Comment 7 Tom Hughes 2020-09-01 15:07:44 UTC
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.

Comment 8 Tom Hughes 2020-09-01 16:08:34 UTC
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.

Comment 9 Tom Hughes 2020-09-01 19:54:14 UTC
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.

Comment 10 Avi Kivity 2020-10-04 15:10:38 UTC
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.

Comment 11 Fedora Update System 2020-10-05 17:53:26 UTC
FEDORA-2020-3307ecc210 has been submitted as an update to Fedora 32. https://bodhi.fedoraproject.org/updates/FEDORA-2020-3307ecc210

Comment 12 Fedora Update System 2020-10-06 01:25:15 UTC
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.

Comment 13 Avi Kivity 2020-10-06 06:32:01 UTC
I confirm the Fedora 32 update works. Thanks.

Comment 14 Fedora Update System 2020-10-13 20:34:55 UTC
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.


Note You need to log in before you can comment on or make changes to this bug.