Skip to content

Conversation

@b1f6c1c4
Copy link
Contributor

@b1f6c1c4 b1f6c1c4 commented Feb 23, 2021

C++20 has significant modifications on the overloads of operator==. See the following demo: https://godbolt.org/z/dTvncj

In order to ease the transition to C++20 some day, it'd be better to stop relying on the C++17's operator== behavior; otherwise, the following error would occur when compiling the existing code base using C++20:

../test/networks/xag.cpp:264:19: error: ambiguous overload for ‘operator==’ (operand types are ‘const mockturtle::node_pointer<1>’ and ‘const mockturtle::xag_network::signal’)
  264 |       CHECK( node == f1 ); /* first po */

@b1f6c1c4
Copy link
Contributor Author

Besides, it would be better to add another item(s) into .github/workflows/*.yml that verifies C++20-compliance of the source code. It may not be necessary to change the global CMakeList.txt to C++20 at this moment, IMO.

@coveralls
Copy link

coveralls commented Feb 23, 2021

Pull Request Test Coverage Report for Build 599328370

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.008%) to 79.787%

Totals Coverage Status
Change from base Build 592722214: 0.008%
Covered Lines: 10496
Relevant Lines: 13155

💛 - Coveralls

@hriener
Copy link
Member

hriener commented Feb 24, 2021

Thank you for the PR!

As you have noticed, we do not support C++20 yet. However, I appreciate the changes to prepare mockturtle for the future.

Do you have a suggestion for the workflow update? What's the compiler that you would like to target?

@b1f6c1c4
Copy link
Contributor Author

@hriener I believe GCC >=10.2.0 would do the job (I'm personally using 10.2.0 locally.)
Besides, I think passing some parameters to the cmake command line in somewhere like https://github.com/lsils/mockturtle/blob/master/.github/workflows/linux.yml#L24 would be beneficial.

@hriener
Copy link
Member

hriener commented Feb 24, 2021

Agreed. I'm happy to merge the workflow/CMake update if you would like to commit the changes to the branch (assuming that the required changes are not too invasive).

@b1f6c1c4
Copy link
Contributor Author

I'm not very familiar with github workflows so maybe you could merge the PR first and the modify .github/workflows/*.yml later.

@hriener
Copy link
Member

hriener commented Feb 24, 2021

I like your idea of adding a c++20 compiler first to verify that the changes are indeed required and correct. Also, updating the workflows should be not too complicated; let me see if I can get it done by the end of the week.

@hriener hriener merged commit 765322f into lsils:master Feb 25, 2021
@hriener
Copy link
Member

hriener commented Feb 25, 2021

Thanks!

@hriener hriener mentioned this pull request Feb 25, 2021
@b1f6c1c4 b1f6c1c4 deleted the patch-2 branch February 25, 2021 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants