refactor: Abstract boost::variant out #17953
Conversation
| template <typename T> | ||
| void operator==(const T&) const | ||
| { | ||
| static_assert(false && sizeof(T), "Compared a Variant directly with type T. this prevented an implicit conversion"); |
There was a problem hiding this comment.
for reviewers, what's the sizeof(T) trick here?
There was a problem hiding this comment.
right forgot to mention that.
if I just do static_assert(false, msg) it won't compile no matter what.
now because the sizeof(T) is here then it can't evaluate the expression without monomorphising the function so it can know what T is. so as long as this function is never called the compiler won't fail compilation because it will never be able to evaluate it.
ie without: https://godbolt.org/z/ZGAFQB (fail compilation even though it isn't being called)
with: https://godbolt.org/z/JCDeeM (doesn't fail because can't evaluate)
with and used: https://godbolt.org/z/KCfNSV (fail compilation)
FYI I stole that trick from: https://www.boost.org/doc/libs/1_71_0/boost/variant/variant.hpp (see BOOST_STATIC_ASSERT)
elichai
force-pushed
the
2020-01-variant
branch
2 times, most recently
from
3f10031 to
497a773
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 497a77358ff5a3baa62b96d8d83a8edf5d72ddd1
src/Makefile.am
Outdated
There was a problem hiding this comment.
This seems consistent since our optional and span replacements are also at the top level. But I wonder if they should all move to src/util/ or some place to keep the top level less crowded. Probably something to deal with in a different PR
elichai
force-pushed
the
2020-01-variant
branch
from
497a773 to
08bf1e5
Compare
|
|
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
Code review ACK 08bf1e5e213a5926a41433b6c532867aca48ca12. Only change since last review is adding and removing some constructor and assignment overloads
Not sure what's the problem in s390x, https://travis-ci.org/bitcoin/bitcoin/jobs/641327078 seems to be linker related.
It looks like the build is just timing out during the link step and being terminated. I'd guess it's not related to this PR.
elichai
force-pushed
the
2020-01-variant
branch
from
08bf1e5 to
86870ff
Compare
|
Not sure what the point of this is, but aren't we going to switch to c++17 soon anyway? In that case we could just replace it once with the std::variant instead of first wrapping it and then switching it around in the wrapper. |
The point is to lay the ground by not depending directly on boost everywhere, and also making it more obvious when I think that's the same goal that |
|
|
||
| #include <node/transaction.h> | ||
| #include <outputtype.h> | ||
| #include <variant.h> |
There was a problem hiding this comment.
Alphabetical order of includes?
| @@ -0,0 +1,153 @@ | |||
| // Copyright (c) 2017-2019 The Bitcoin Core developers | |||
| #define BITCOIN_VARIANT_H | ||
|
|
||
| #include <utility> | ||
| #include <type_traits> |
There was a problem hiding this comment.
Alphabetical order of includes?
| Needs rebase |
|
Checking in on the value of going forward with this since C++17 is ~5 months away and there is still some work to be done to get this merged (rebase). |
I guess other reviewers have to weight in, but I'm happy with the approach here and would re-ack if rebased. I think this PR still makes sense with even an upcoming transition to std::variant, because it brings existing variant code into alignment with the std::variant API so fewer changes to this code are needed later, even though some temporary util code is added now |
It does not. The boost::variant and std::variant APIs are not identical. The API variant that is introduced in this pull is identical to neither of the "official" ones. So if this is merged, we will end up with three APIs to keep in mind and maintain. I just don't see the value of spending time on review when the changes here can be done (in theory) with a trivial scripted diff. See https://github.com/MarcoFalke/bitcoin-core/pull/new/2005-StdVariantScriptedDiff The changes here are already complicated and they even miss the most complicated part (boost visitor). Feel free to disagree, but going forward, I am hoping to see something like #18863 merged for 0.21, so that the diff in my branch is reduced for 0.22. |
|
To compare the two branches:
|
|
That all makes sense. So it sounds like even though this PR has some merits, it conflicts with better alternatives and may not be worth effort to review and rebase now and update for future changes later. |
9 participants
Hi,
This introduces a level of abstractions for
boost::variantsuch that when we switch to C++17 we can drop instd::variantinstead and everything should work modulo the visitors.The reason I went for a class and not a typedef/using like with
Optionalis because I want to add thegetfunction so "users" won't need to still includeboost/variant/get.hppand because I wanted to make it more explicit whengetis throwing and when not (by splitting togetandget_iflike stl does).I also replaced the generic(Had to remove this since it broke a build, not sure if it's because a specific boost version or a clang version https://travis-ci.org/bitcoin/bitcoin/jobs/638552502)boost/variant.hppwithboost/variant/get.hppandboost/variant/variant.hppwhich are just 2 out of the 7 headers inboost/variant.hpp(https://www.boost.org/doc/libs/1_71_0/boost/variant.hpp)