-
Notifications
You must be signed in to change notification settings - Fork 447
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Workaround for gcc-11.4/draft 2x spec flaw #4679
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I did not realize this is GCC 11 specific problem. Is this enough to compile P4C in C++20 mode?
@@ -130,7 +130,7 @@ class NameMap : public Node { | |||
} | |||
|
|||
IRNODE_SUBCLASS(NameMap) | |||
bool operator==(const Node &a) const override { return a == *this; } | |||
bool operator==(const Node &a) const override { return a.operator==(*this); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment for this one why it is needed. This is not obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its also unfortunately not easy to explain, but is the core of the issue.
C++20 adds the ability to resolve overload of relational operators with the operands swapped -- so a == b
can call either a.operator==(b)
or b.operator==(a)
, and the flaw in the early c++2x draft meant that this specific function would end up calling itself recursively. Using an explicit operator==
forces it to use that version and not the reversed version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great. Let's see whether we can make progress on the C++20 port with this.
One conclusion from this PR is that it could be a bit tricky to test if we move to C++20 as various compilers have various incomplete and not entirely correct implementations. I'd suggest testing at least with GCC 9 (as that is our minimum), GCC 11 (Ubuntu 22 and it also has this problem) and GCC 13 (Ubuntu 24). But we probably should have CI for all of these anyway... |
We might upgrade our minimum to 10 considering it is now the default gcc for Ubuntu 20.04?
It might be hard to make guarantees beyond what we have running in our CI. We could also consider raising the minimum required GCC version to 10. It is installable in Ubuntu 20.04 In any case, Ubuntu 20.04 will hit EOL next year. We could shift to Ubuntu 22.04/24 split and explicitly support the compilers provided by those distributions. |
e9e277e
to
0350d5b
Compare
- A late draft of the C++2x spec introduced a non-backwards compatible change in the way overloading for operator ==/!= is handled; this was considered a defect and was fix in the final C++20 spec.
TLDR: workaround for problems compiling p4c with gcc 11.4 (and probably others) with
-std=c++2x
A late draft of the C++2x spec introduced a non-backwards compatible change in the way overloading for operator ==/!= is handled; this was considered a defect and was fixed in the final C++20 spec. However, the default version of gcc on Ubuntu 22.04 (gcc 11.4) happens to implement that spec draft, so gives errors with our code.
More recent versions of gcc (and older versions too!) don't have this problem. So this change isn't really necessary, but is pretty small and may help others who run into this specific problem.