refactor: Abstract boost::variant out

elichai commented at 6:01 pm on January 17, 2020: contributor

Hi, This introduces a level of abstractions for boost::variant such that when we switch to C++17 we can drop in std::variant instead and everything should work modulo the visitors.

The reason I went for a class and not a typedef/using like with Optional is because I want to add the get function so “users” won’t need to still include boost/variant/get.hpp and because I wanted to make it more explicit when get is throwing and when not (by splitting to get and get_if like stl does).

I also replaced the generic boost/variant.hpp with boost/variant/get.hpp and boost/variant/variant.hpp which are just 2 out of the 7 headers in boost/variant.hpp(https://www.boost.org/doc/libs/1_71_0/boost/variant.hpp) (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)

in src/variant.h:124 in 278dc53fa8 outdated

125+    }
126+    // This are here to prevent implicit conversion to Variant and throw a compile time error.
127+    template <typename T>
128+    void operator==(const T&) const
129+    {
130+        static_assert(false && sizeof(T), "Compared a Variant directly with type T. this prevented an implicit conversion");

instagibbs commented at 6:11 pm on January 17, 2020:

for reviewers, what’s the sizeof(T) trick here?

elichai commented at 6:17 pm on January 17, 2020:

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)

DrahtBot added the label Build system on Jan 17, 2020

DrahtBot added the label GUI on Jan 17, 2020

DrahtBot added the label Refactoring on Jan 17, 2020

DrahtBot added the label RPC/REST/ZMQ on Jan 17, 2020

DrahtBot added the label Tests on Jan 17, 2020

DrahtBot added the label Wallet on Jan 17, 2020

fanquake removed the label Build system on Jan 17, 2020

fanquake removed the label GUI on Jan 17, 2020

fanquake removed the label RPC/REST/ZMQ on Jan 17, 2020

fanquake removed the label Tests on Jan 17, 2020

fanquake removed the label Wallet on Jan 17, 2020

elichai force-pushed on Jan 17, 2020

DrahtBot commented at 0:58 am on January 18, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18202 (refactor: consolidate sendmany and sendtoaddress code by Sjors)
#18194 (Bugfix: GUI: Remove broken ability to edit the address field in the sending address book by luke-jr)
#18115 (wallet: Pass in transactions and messages for signing instead of exporting the private keys by achow101)
#17938 (Disallow automatic conversion between disparate hash types by Empact)
#17623 (rpc: add extensive file checks for dumptxoutset and dumpwallet by brakmic)
#17577 (refactor: deduplicate the message sign/verify code by vasild)
#16549 ([WIP] UI external signer support (e.g. hardware wallet) by Sjors)
#16546 ([WIP] External signer support - Wallet Box edition by Sjors)
#16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
#16440 (BIP-322: Generic signed message format by kallewoof)
#16378 ([WIP] The ultimate send RPC by Sjors)
#15294 ([moveonly] wallet: Extract RipeMd160 by Empact)
#15115 (GUI: Replace send-to-self with dual send+receive entries by luke-jr)

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.

fanquake requested review from ryanofsky on Jan 23, 2020

in src/variant.h:31 in 85882c01d2 outdated

26+        m_var = other.m_var;
27+    }
28+    Variant(Variant& other)
29+    {
30+        m_var = other.m_var;
31+    }

ryanofsky commented at 7:29 pm on January 23, 2020:

In commit “Add a new Variant type” (85882c01d20add49f7b1f8b741e078968ec8080b)

Maybe drop this overload. I don’t think it’s doing anything the other two overloads don’t cover. Also std::variant doesn’t have this: https://en.cppreference.com/w/cpp/utility/variant/variant

elichai commented at 7:58 pm on January 23, 2020:

You’re right, I was sure I dropped it. There’s no reason for both const and non-const constructor if it doesn’t mutate.

elichai commented at 9:11 pm on January 23, 2020:

I see now why I didn’t drop it :) Because then the following code won’t compile:

0Variant<int> var = 5;
1Variant<int> a = var;

Because Variant& operator=(T&& value) signature can match it “better” then Variant(const Variant& other) so it will choose that and then it will try to handle it like a value instead of a variant which will fail to compile (https://gcc.godbolt.org/z/BKzBkE) not quite sure how libstd makes it work (I think it can be done with std::enable_if but that’s uglier than another constructor)

ryanofsky commented at 9:31 pm on January 23, 2020:

I see now why I didn’t drop it :)

Makes sense, I see why it’s there now

elichai commented at 10:05 pm on January 23, 2020:

How would you feel about removing this and adding enable_if to all the templated versions that checks its not of type Variant?

The tradeoff here is more specialized constructors versus less readable templates

ryanofsky commented at 10:15 pm on January 23, 2020:

How would you feel about removing this and adding enable_if to all the templated versions that checks its not of type Variant?

The tradeoff here is more specialized constructors versus less readable templates

Either way seems good to me. You should choose which way you like best / hate least. I guess it’s a choice between defining more repetitive constructors, or being more concise but using enable_if.

elichai commented at 12:18 pm on January 24, 2020:

Done

in src/variant.h:40 in 85882c01d2 outdated

35+    }
36+    template <class T>
37+    Variant(const T& value)
38+    {
39+        m_var = value;
40+    }

ryanofsky commented at 7:32 pm on January 23, 2020:

In commit “Add a new Variant type” (85882c01d20add49f7b1f8b741e078968ec8080b)

I think this overload is also not doing anything

elichai commented at 8:01 pm on January 23, 2020:

Hmm seems that boost::variant implements this but std::variant doesn’t. doesn’t this act as a copy constructor for a T?

ryanofsky commented at 8:23 pm on January 23, 2020:

Hmm seems that boost::variant implements this but std::variant doesn’t. doesn’t this act as a copy constructor for a T?

I think boost variant might implement this to deal with pre-c++11 compilers that don’t support rvalue references. But the overload below template <class T> Variant(T&& value) should make this redundant with a c++11 compiler.

elichai commented at 12:18 pm on January 24, 2020:

Done

in src/variant.h:44 in 85882c01d2 outdated

39+        m_var = value;
40+    }
41+    template <class T>
42+    Variant(T&& value)
43+    {
44+        m_var = std::move(value);

ryanofsky commented at 7:34 pm on January 23, 2020:

In commit “Add a new Variant type” (85882c01d20add49f7b1f8b741e078968ec8080b)

Maybe want std::forward<T>(value) instead of std::move(value) because this constructor (because it’s a template) can be called with lvalues where it would be surprising to move from.

sipa commented at 8:02 pm on January 23, 2020:

~~If you turn this into an std::forward I think you can drop all other copy constructors.~~

Nevermind, the type doesn’t match of course.

elichai commented at 8:17 pm on January 23, 2020:

I didn’t really look into std::forward before, I now think I understand how and why it is useful, pretty cool. Thanks.

elichai commented at 12:18 pm on January 24, 2020:

Done

in src/variant.h:71 in 85882c01d2 outdated

73+    Variant& operator=(const Variant& other)
74+    {
75+        m_var = other.m_var;
76+        return *this;
77+    }
78+    Variant& operator=(Variant&& other)

ryanofsky commented at 7:41 pm on January 23, 2020:

In commit “Add a new Variant type” (85882c01d20add49f7b1f8b741e078968ec8080b)

Maybe add a template operator=, too. https://en.cppreference.com/w/cpp/utility/variant/operator%3D shows this as the third overload for operator=

elichai commented at 8:23 pm on January 23, 2020:

seems that also here(like the constructor) boost adds both rlvaue and lvalue operators. i’ll add just the rvalue like std::variant does. (although I don’t quite understand why only that)

ryanofsky commented at 8:30 pm on January 23, 2020:

seems that also here(like the constructor) boost adds both rlvaue and lvalue operators. i’ll add just the rvalue like std::variant does. (although I don’t quite understand why only that)

The reason why only a T&& overload is needed is that when T is a template type, there are template deduction rules that let T be deduced to be an lvalue reference, so the function can be called with both lvalue and rvalue arguments. (This is needed for “perfect forwarding”). If T was a normal type and not a template type, you would need to have both T& and T&& overloads to accept lvalues and rvalues.

elichai commented at 9:20 pm on January 23, 2020:

argh This also introduces the same problem as in #17953 (review)

elichai commented at 12:18 pm on January 24, 2020:

Done

ryanofsky approved

ryanofsky commented at 7:45 pm on January 23, 2020: member

Code review ACK 497a77358ff5a3baa62b96d8d83a8edf5d72ddd1

in src/Makefile.am:234 in 85882c01d2 outdated

230@@ -231,6 +231,7 @@ BITCOIN_CORE_H = \
231   util/vector.h \
232   validation.h \
233   validationinterface.h \
234+  variant.h \

ryanofsky commented at 9:35 pm on January 23, 2020:

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

MarcoFalke commented at 9:55 pm on January 23, 2020:

See also #15732

ryanofsky approved

elichai force-pushed on Jan 24, 2020

elichai commented at 1:59 pm on January 24, 2020: contributor

~~Not sure what’s the problem in s390x, https://travis-ci.org/bitcoin/bitcoin/jobs/641327078 seems to be linker related.~~ (Not happening anymore, CI related)

in src/variant.h:34 in 2eacf26ff6 outdated

29+    Variant(Variant&& other)
30+    {
31+        m_var = std::move(other.m_var);
32+    }
33+    // Require that typeof(T) != Variant. to prevent wrong template instantiations
34+    template <class T, typename = typename std::enable_if<!std::is_same<typename std::remove_reference<T>::type, Variant>::value>::type>

ryanofsky commented at 7:54 pm on January 24, 2020:

In commit “Add a new Variant type” (2eacf26ff61a63fce088600ced16ec2d4d5d771d)

Might be a little better to replace std::remove_reference with std::decay to remove CV qualifiers as well

elichai commented at 11:18 am on January 26, 2020:

ah ha! I looked for the closest thing to C++20’s std::remove_cvref looks like decay is the better option

ryanofsky approved

ryanofsky commented at 7:59 pm on January 24, 2020: member

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.

Add a new Variant type c901f091a3

Replace boost::variant with Variant in variant.h 86870ff053

elichai force-pushed on Jan 26, 2020

ryanofsky approved

ryanofsky commented at 10:37 pm on January 27, 2020: member

Code review ACK 86870ff053f58f9f0a3ca4ca7ba648c6b1572f49. Only change since last review is switching to std::decay

MarcoFalke commented at 8:55 am on January 31, 2020: member

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.

elichai commented at 12:29 pm on January 31, 2020: contributor

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 get and get_if is used. that way the actual removal of boost::variant is just 2-3 lines in this file.

I think that’s the same goal that fs.h and optional.h service.

in src/rpc/util.h:10 in 86870ff053

 6@@ -7,6 +7,7 @@
 7 
 8 #include <node/transaction.h>
 9 #include <outputtype.h>
10+#include <variant.h>

kristapsk commented at 9:47 pm on February 22, 2020:

Alphabetical order of includes?

in src/variant.h:1 in 86870ff053

0@@ -0,0 +1,153 @@
1+// Copyright (c) 2017-2019 The Bitcoin Core developers

kristapsk commented at 9:47 pm on February 22, 2020:

2020?

in src/variant.h:9 in 86870ff053

0@@ -0,0 +1,153 @@
1+// Copyright (c) 2017-2019 The Bitcoin Core developers
2+// Distributed under the MIT software license, see the accompanying
3+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+#ifndef BITCOIN_VARIANT_H
6+#define BITCOIN_VARIANT_H
7+
8+#include <utility>
9+#include <type_traits>

kristapsk commented at 9:50 pm on February 22, 2020:

Alphabetical order of includes?

DrahtBot added the label Needs rebase on Feb 25, 2020

DrahtBot commented at 11:18 am on February 25, 2020: member

adamjonas commented at 1:06 am on May 2, 2020: member

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).

ryanofsky commented at 8:48 pm on May 13, 2020: member

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

MarcoFalke commented at 10:07 pm on May 13, 2020: member

it brings existing variant code into alignment with the std::variant API

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.

MarcoFalke commented at 10:13 pm on May 13, 2020: member

To compare the two branches:

2005-StdVariantScriptedDiff: 107 additions and 127 deletions (removes boost completely in one go)
2020-01-variant: Adds a file with 155 lines + other changes (is only the first step to remove boost, needs follow-ups and eternal maintenance)

ryanofsky commented at 10:24 pm on May 13, 2020: member

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.

elichai closed this on May 14, 2020

DrahtBot locked this on Feb 15, 2022

refactor: Abstract boost::variant out #17953

Conflicts