--- Day changed Wed Jun 24 2020 00:32 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 00:35 -!- givanse [~givanse@cpe-23-243-162-78.socal.res.rr.com] has quit [Ping timeout: 256 seconds] 00:37 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 00:54 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined #bitcoin-core-pr-reviews 01:30 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 01:33 -!- seven_ [~seven@2a00:ee2:410c:1300:383b:4ec2:aff:8e92] has quit [Read error: Connection reset by peer] 01:43 -!- belcher [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 01:50 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined #bitcoin-core-pr-reviews 01:54 -!- S3RK [~s3rk@47.246.66.115] has quit [Remote host closed the connection] 02:05 -!- jonatack [~jon@37.173.234.43] has quit [Ping timeout: 264 seconds] 02:05 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Ping timeout: 240 seconds] 02:08 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 02:15 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 02:19 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 265 seconds] 02:21 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined #bitcoin-core-pr-reviews 02:52 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 02:56 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 240 seconds] 03:01 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 03:05 -!- Vicente85Sporer [~Vicente85@static.57.1.216.95.clients.your-server.de] has joined #bitcoin-core-pr-reviews 03:10 -!- Vicente85Sporer [~Vicente85@static.57.1.216.95.clients.your-server.de] has quit [Ping timeout: 260 seconds] 03:27 -!- jonatack [~jon@37.173.234.43] has joined #bitcoin-core-pr-reviews 03:28 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 03:34 -!- jonatack [~jon@37.173.234.43] has quit [Ping timeout: 240 seconds] 03:36 -!- jonatack [~jon@37.173.234.43] has joined #bitcoin-core-pr-reviews 03:42 -!- jonatack [~jon@37.173.234.43] has quit [Read error: Connection reset by peer] 03:43 -!- jonatack_ [~jon@37.173.234.43] has joined #bitcoin-core-pr-reviews 03:45 -!- jonatack_ [~jon@37.173.234.43] has quit [Client Quit] 03:46 -!- jonatack [~jon@2a01:e0a:53c:a200:bb54:3be5:c3d0:9ce5] has joined #bitcoin-core-pr-reviews 03:51 -!- reallll [~belcher@unaffiliated/belcher] has joined #bitcoin-core-pr-reviews 03:54 -!- belcher [~belcher@unaffiliated/belcher] has quit [Ping timeout: 240 seconds] 04:02 -!- reallll is now known as belcher 04:38 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has joined #bitcoin-core-pr-reviews 04:52 < raj_> thanks sipa for your detailed answers. :) learnt a lot in this PR. 04:52 -!- S3RK [~s3rk@47.246.66.115] has quit [Remote host closed the connection] 05:29 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 05:36 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 260 seconds] 05:37 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 05:41 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 240 seconds] 06:30 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 06:37 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 258 seconds] 07:15 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has quit [Read error: Connection reset by peer] 07:16 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has joined #bitcoin-core-pr-reviews 07:28 -!- slivera [~slivera@103.231.88.30] has quit [Remote host closed the connection] 07:34 -!- tryphe_ [~tryphe@unaffiliated/tryphe] has joined #bitcoin-core-pr-reviews 07:35 -!- tryphe [~tryphe@unaffiliated/tryphe] has quit [Ping timeout: 240 seconds] 07:46 -!- randon [6da685bb@109.166.133.187] has joined #bitcoin-core-pr-reviews 08:02 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 08:16 -!- pinheadmz1 [~pinheadmz@pool-100-33-69-78.nycmny.fios.verizon.net] has joined #bitcoin-core-pr-reviews 08:17 < pinheadmz1> Never tried to actually compile bitcoin-qt before. Looks like it worked, but can't figure out how to launch the GUI applciation in ubuntu. src/qt/bitcoin-qt won't just run 08:18 < pinheadmz1> ok wait it will if i command it from terminal, just not on double click 08:29 < raj_> jonatack that would be a really great timing for me (India) but thats also my dayjob hours. Its a weird problem which many of us face who don't do this full time. Mostly we rise at night times to do bitcoin stuffs. 08:31 -!- omidbarat [~omidbarat@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has joined #bitcoin-core-pr-reviews 08:31 < omidbarat> hello 08:32 -!- omidbarat [~omidbarat@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has quit [Client Quit] 08:34 -!- fodediop [~fodediop@41.214.84.170] has joined #bitcoin-core-pr-reviews 08:35 -!- tralfaz [~tralfaz@68.235.43.150] has joined #bitcoin-core-pr-reviews 08:35 -!- fodediop [~fodediop@41.214.84.170] has quit [Client Quit] 08:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 08:42 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 08:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 08:44 -!- vasild_ is now known as vasild 08:46 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 240 seconds] 08:47 -!- randon [6da685bb@109.166.133.187] has quit [Ping timeout: 245 seconds] 09:11 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Remote host closed the connection] 09:11 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews 09:15 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Ping timeout: 240 seconds] 09:19 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 09:25 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 09:33 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 09:33 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has quit [Client Quit] 09:35 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 09:41 -!- hashbasher [~omidbarat@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has joined #bitcoin-core-pr-reviews 09:43 -!- hashbasher [~omidbarat@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has quit [Client Quit] 09:45 -!- hashbasher [~hashbashe@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has joined #bitcoin-core-pr-reviews 09:47 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-hvovjybxblwxfqzh] has quit [Ping timeout: 244 seconds] 09:59 -!- jules69 [4486dbb5@pool-68-134-219-181.bltmmd.fios.verizon.net] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> #startmeeting 10:00 -!- gimballock [b8a4b1b9@d-184-164-177-185.fl.cpe.atlanticbb.net] has joined #bitcoin-core-pr-reviews 10:00 < fjahr> hi 10:00 < felixweis> hi 10:00 < willcl_ark> hi 10:00 < gimballock> hi 10:00 < jnewbery> welcome to review club everyone! Feel free to say hi to let everyone know you're here. 10:00 < hashbasher> 'ello 10:00 -!- yiannix [~yiannix@2a01:4b00:850f:4500:3cfd:2424:d8bb:4ad2] has joined #bitcoin-core-pr-reviews 10:00 < jnewbery> (or just lurk. lurkers welcome here too) 10:00 < emzy> hi 10:00 < michaelfolkson> hi 10:01 < jnewbery> Today we're looking at PR 18468 (Span improvements), although I expect the discussion might touch on other aspects of Spans that aren't specifically in that PR. 10:01 < jnewbery> Notes and questions are in the usual place: https://bitcoincore.reviews/18468.html 10:01 < jkczyz> hi 10:01 < jnewbery> sipa has very kindly offered to host today. He also contributed all of our Span implementation in Bitcoin Core, so he should be able to answer at least some of our questions :) 10:01 < jnewbery> Over to you, sipa 10:01 < raj_> hi 10:01 < sipa> hi 10:02 < sipa> so, today is a bit of an unusual review club, as there is very little bitcoin-specific knowledge involved 10:02 < troygiorshev> hi 10:02 < nehan_> hi 10:02 < sipa> but that's well compensated by needing some nitty gritty details of C++ templates instead... 10:02 < sipa> so, who has had a chance to review the PR? 10:03 < raj_> y 10:03 < fjahr> y 10:03 < troygiorshev> n 10:03 < nehan_> n 10:03 < willcl_ark> y 10:03 < jkczyz> n - mostly lurking today but find the topic interesting 10:03 < emzy> y/n 10:03 < jnewbery> y 10:03 < felixweis> n still looking at the history of span 10:04 < sipa> perhaps some additional last-minute reference material: https://github.com/bitcoin/bitcoin/pull/19367 10:04 -!- clarify [~clarify@178.162.222.42.adsl.inet-telecom.org] has joined #bitcoin-core-pr-reviews 10:04 < sipa> inspired by things pointed out in the PR, as well as explaining some of its uses 10:05 < sipa> so the first real question: Do you think Span is a useful abstraction? Can you think of more places where it could be used to simplify existing code? 10:05 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has joined #bitcoin-core-pr-reviews 10:05 < amiti> I found 19367 very helpful. thanks for adding! 10:05 < fjahr> Definitely useful but I couldn't come up with good places to use it yet. Anything where we read a lot of data from disk I suspect. 10:06 -!- ecurrencyhodler [68ac287f@cpe-104-172-40-127.socal.res.rr.com] has joined #bitcoin-core-pr-reviews 10:06 < jnewbery> +1 19367 is very helpful 10:06 < nehan_> I'd like to understand when a span is better than a pointer or reference to the original datastructure 10:06 < michaelfolkson> Can we quickly first just confirm the motivation for Span? We are concerned with buffer overflows right? And including a size with a pointer ensures these don't occur? 10:06 < willcl_ark> I couldn't tell if Span was improving program performance, or just providing a friendlier interface for the programmer 10:07 < raj_> I am still tumbling over the usefulness part. Can someone explain how is it better than other collection types? 10:07 < jnewbery> willcl_ark: I had the same question. Is the motivation code clarity/safety or performance 10:07 < sipa> raj_: that one is easy to answer: a Span is not a collection! 10:07 < sipa> raj_: it is a reference to a range of elements, but it's cheap to copy/modify/pass around 10:08 < sipa> in a way, it's an alternative (in some cases) to always passing around a pointer/length pair, or a begin iterator/end iterator pair 10:08 < sipa> so when compared to that, i'd say it's a better interface, that's more readable and less prone to errors 10:08 < sipa> but as it's literally just encapsulating a pointer and a length, there is no performance difference to doing that instead 10:09 < jnewbery> right, it doesn't own the data, so it's up to the programmer to not shoot themself in the foot by doing something like using after free 10:09 < willcl_ark> ok, thanks 10:09 < nehan_> sipa: when is it important to have the length/end iterator? 10:09 < andrewtoth> so it is similar to a tuple in other languages? 10:09 < sipa> andrewtoth: no 10:09 < sipa> that'd be std::tuple 10:09 < nehan_> it reminds me a little of a slice in go 10:09 < andrewtoth> ahh thanks 10:09 < sipa> nehan_: i believe it is similar to that 10:10 < sipa> i've seen C++ codebases that had their own span like type named Slice 10:10 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Remote host closed the connection] 10:10 < nehan_> however it's super scary because updates to the original object might invalidate the span 10:10 < sipa> as a motivation, let me link to this: https://github.com/bitcoin/bitcoin/pull/19326 10:10 < jnewbery> it could also be called a view 10:11 < sipa> just scrolling through it, you can see dozens of places where we compute a begin and end iterator, and pass both into a function that hashes 10:11 < jnewbery> (in fact, I think the original c++ proposal was called array_view) 10:11 < raj_> +1 jnewbery view seems like a better name to express its nature. specially the scarry parts. 10:12 < sipa> there is a "const char" specific span like object too introduced in c++17, called string_view 10:12 < sipa> which is very similar, but has some string-like operations on it defined for convenience 10:13 < emzy> jnewbery: array_view builds a better image im my mind as slice or sapn. tnx. 10:13 < sipa> michaelfolkson: i wouldn't say that preventing buffer overdlows is the primary motivation... indirectly i expect that more readable code for working with ranges of objects reduces the chance of that, but i'd say readability is the goal 10:14 < willcl_ark> seems also (a little bit!) like a Python memoryview in that case, although not only for streams 10:14 < sipa> fjahr: unsure what disk access has to do with it, as spans always refer to ranges of elements *in memory* 10:15 -!- salvatoshi [~salvatosh@2001:b07:644b:1d18:d9ba:58cc:b4b:b7b3] has joined #bitcoin-core-pr-reviews 10:15 -!- figs [2f293fee@047-041-063-238.res.spectrum.com] has joined #bitcoin-core-pr-reviews 10:15 < hashbasher> +1 nehan's question, when's it important to know the lenght/end iterator? 10:16 < emzy> span is very generic, for every object type? 10:16 < sipa> emzy: indeed 10:16 < michaelfolkson> sipa: Thanks. So is this use case of making the script interpreter independent from CScript possible without Span? Span just makes it easier due to more readable code? 10:16 < sipa> michaelfolkson: exactly 10:16 < michaelfolkson> Cool 10:16 < sipa> for example look at https://github.com/bitcoin/bitcoin/blob/master/src/script/interpreter.cpp#L1525L1558 10:17 < sipa> the script interpreter for witness scripts, it has to deal with a range of elements that are the actual stack, and then a final one that is the script 10:17 < willcl_ark> It certainly seems handy for the experienced programmer, but also seems to introduce some "hidden" pitfalls (which are described in #19367) that you might be more inclined to consider with a pointer/length pair. 10:17 < sipa> instead of code that does iterator arithmetic to keep track of everything 10:18 < willcl_ark> Does it improve performance at all avoiding computing begin/end iterators? 10:18 < fjahr> sipa: I was just thinking of at which points we might want to handle data efficiently and I thought it might be connect with data that gets saved to disk sometimes. But yes, it does not directly make the case to use a span. 10:18 < sipa> you build a span of all the elements, and then extract subspans 10:18 < sipa> hashbasher: say you have an array of 20 bytes, and want to hash them 10:18 < jnewbery> ok, here's an attempt at motivation. sipa, let me know if this is off: 10:19 < sipa> the typical way would be to call a hash function, which you pass a pointer to the beginning of the array, plus the length 10:19 < jnewbery> Often, we'll have data stored in continguous range-like contains (arrays, vactors, ...). There are functions that we want to call with that data, but those functions don't want to care about the specifics of the container, so currently they take a pointer and a size, which the caller needs to fish out of the container. 10:19 < jnewbery> With a Span, and implicit conversion from those containers to Span, the function can just take a Span, the caller can pass any of those range-like containers, and the conversion will take care of it for you. 10:19 < sipa> jnewbery: yeah, good to bring that up - it's a great use case but not the only one 10:19 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eqdnsxwkcwlcrcwi] has joined #bitcoin-core-pr-reviews 10:19 < sipa> it's a way of writing functions that operate generically on whatever type of container your data is stored in 10:20 < sipa> and an anti-pattern that sometimes emerges is "oh, let's make the function only accept a vector, and if someone has it in some other form, they can construct a vector with a copy of the data" - which is obviously wasteful 10:21 -!- vasild [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 10:21 < jnewbery> wasteful and wouldn't work if you wanted to do something non-const with the data 10:21 < sipa> with a span you'd write the function once, and it'll work when called with a vector, or an array, or an std::array, or a prevector, or a uint256, or a CPubKey (the latter two also being types that act as ranges bytes!) 10:22 < willcl_ark> OK that's definitely useful 10:22 < hashbasher> +1 makes much more sense to me now 10:22 < nehan_> sipa: why span instead of generics? 10:23 < sipa> nehan_: by generics you mean templates? 10:23 < nehan_> yes 10:23 < sipa> good question 10:23 < sipa> one is that it forces instantiation at compile time for every type you invoke it with 10:24 < sipa> which may be worth it, but in many cases it's also not: if literally all your function needs is a range, there is no reason to instantiate it for vector and prevector and array and ... 10:24 < sipa> another is that spans support range-like operations, like subspans 10:24 < sipa> which is e.g. heavily relied on in the descriptor parsing code 10:24 -!- Talkless [~Talkless@hst-227-49.splius.lt] has joined #bitcoin-core-pr-reviews 10:25 < sipa> which passes down smaller and smaller spans to the string to be parsed, as it gets dissected 10:25 < jnewbery> the downside of instatiating for all types is marginally longer compile times and marginally larger binary size? Anything else? 10:26 < sipa> you couldn't do that with templated functions - you'd still need to create copies any time you want to pass down a substring 10:26 < sipa> jnewbery: yeah 10:26 < sipa> also forcing everything to be in headers 10:26 < jnewbery> ah yes 10:26 < MarcoFalke> sipa: You could (in theory) put all template instantiations in the cpp file 10:26 < MarcoFalke> But that requires knowing them upfront 10:26 < sipa> MarcoFalke: yes, yuck :) 10:27 < sipa> and as said, that doesn't cover all use cases 10:28 < sipa> so perhaps it's reasonable to state it as: a more lightweight alternative to making functions templated over various input types, in case all they need is a range of elements... while simultaneously also supporting passing down sub-ranges 10:28 < sipa> perhaps it's time for the next question? 10:28 < sipa> When reviewing the PR, did you compare with the proposed std::span interface? What differences did you notice? 10:29 < sipa> https://en.cppreference.com/w/cpp/container/span for reference 10:29 < jnewbery> ashamed to say that I did not, even though that question was a pretty strong hint I probably should have 10:29 < raj_> It doesn't have rbegin, rend and empty. 10:29 < fjahr> Extent/size is not a template parameter and we don't have rbegin or rend 10:29 < sipa> very good 10:29 * sipa fishes for more 10:30 < willcl_ark> based on some of the PR comments, it seems like Bitcoin::Span is a subset of std::span 10:30 < sipa> indeed 10:30 < sipa> let's have a look at the constructors 10:30 < sipa> https://en.cppreference.com/w/cpp/container/span/span 10:30 < sipa> in particular the first 2 10:32 < sipa> they take iterators as arguments 10:32 < jnewbery> we don't have a ctor from an iterator? 10:32 < sipa> indeed 10:32 < sipa> any guess why not? 10:32 < jnewbery> Because it's hard to implement without std::address_of 10:32 < sipa> as wumpus just discovered in https://github.com/bitcoin/bitcoin/pull/19373 it'd be pretty useful to have iterator-based constructors 10:33 < jnewbery> according to a code comment I read somewhere :) 10:33 < sipa> what would be the problem with just doing template Span(It begin, It end) { return Span<...>(&*begin, &*end); } 10:34 < instagibbs> guess: you're de-reffing out of bounds memory location in &*end? 10:34 < sipa> instagibbs: bingo 10:34 < sipa> end is likely an iterator to the end of an object, which may not be dereferencable 10:35 < sipa> std::address_of is added in C++20 that lets you get a pointer to the element an iterator refers to, which deferencing it 10:35 < MarcoFalke> Then, why wouldn't Span<...>(&*begin, end-begin) work ( assuming operator& is not overriden)? 10:35 < sipa> MarcoFalke: what if begin is not dereferencable? 10:36 < MarcoFalke> hm :) 10:37 < sipa> you can start special casing the situation where begin==end, and then return a Span(nullptr, 0), and otherwise return Span(&*begin, end-begin) 10:37 < sipa> but that leads to more pitfalls, as the span.data() that comes out would be wrong 10:38 < sipa> anyone have more comments around this? or anything else we've been discussing? 10:38 < willcl_ark> so the difficult case here is an empty array/vector? 10:39 < sipa> willcl_ark: or a span at the end of one (e.g. if you have a vector vec{1,2,3}, then span(vec.end(), vec.end()) is expected to be legal 10:39 < willcl_ark> ah ok 10:39 < raj_> why we are not doing rbegin and rend? or empty by checking if size=0? 10:40 < michaelfolkson> I'm assuming if/when Core updates to C++ 20 these differences won't pose a problem? It will just be unusued functionality? 10:40 < sipa> raj_: why not rbegin/rend? simply because we haven't encountered a use 10:40 < troygiorshev> When would begin not be dereferenceable? naively that doesn't sound like a very useful iterator 10:40 < sipa> troygiorshev: just gave an example Span<...>(vec.end(), vec.end()) is expected to be legal 10:40 < sipa> and begin here is the vector's end 10:41 < MarcoFalke> michaelfolkson: I'd presume when (and if) we upgrade to C++10, then span.h will get deleted and replaced by std::span 10:41 < troygiorshev> sipa: ah thx 10:41 < MarcoFalke> *C++20 10:41 < hashbasher> sipa: can you list a few functional areas of bitcoin core that this change benefits? 10:41 < sipa> michaelfolkson, MarcoFalke: exactly 10:41 < michaelfolkson> MarcoFalke: And it would just slot in easily right? 10:41 < sipa> hashbasher: i've given a list of PRs that make use of Span in the notes 10:41 < sipa> hashbasher: probably best to look at those 10:42 < MarcoFalke> michaelfolkson: jup, because Span claims to be a subset of std::span 10:42 < hashbasher> oh ok, i saw that, didn't go through them 10:42 < sipa> michaelfolkson: yes, it should pretty much be a drop in replacement 10:42 < sipa> ok, next question 10:42 < sipa> What condition is imposed on converting Span into a Span? Why is it useful to permit such conversion, and what are the risks in doing so unconditionally? 10:42 < sipa> this is probably the most advanced C++ question in the series 10:43 -!- petterhs [51a74f5f@95.81-167-79.customer.lyse.net] has joined #bitcoin-core-pr-reviews 10:43 < sipa> but feel free to guess 10:43 < jnewbery> std::is_convertible if the underlying types are convertible then we do it? 10:43 < sipa> which means? 10:43 < fjahr> if implicit conversion between the types is possible we can convert, not sure exactly what can go wrong 10:43 < sipa> not exactly 10:43 < jnewbery> can you implicitly convert from a pointer to an array of type T to a pointer to an array of type C 10:43 < sipa> indeed! 10:44 < sipa> from *an array* of T to *an array* of C 10:44 < sipa> why does this matter? why not just permitting it when a pointer to T can be converted to a pointer to C? 10:44 < jnewbery> important because that implicit conversion means that the objects are of the same size and so pointer arithmetic works as you'd expect 10:44 < sipa> exactly 10:44 -!- dr-orlovsky [~dr-orlovs@xdsl-188-154-186-21.adslplus.ch] has quit [Quit: My MacBook has gone to sleep. ZZZzzz…] 10:44 < sipa> if you have a parent class A, and a child class B 10:45 < sipa> then pointers to B can be converted to pointers to A 10:45 < sipa> but A and B may not have the same size, so after converting, pointer arithmetic may end up in the middle of B's elements 10:46 < sipa> but why do we want such conversion in the first place? 10:46 < jnewbery> I spent a long time scratching my head over the template definitions of the constructors before seeing the comment a few lines below about "pointers to arrays" https://github.com/bitcoin/bitcoin/blob/205b87d2f6bd01285de50ba742e32e4ab1298b13/src/span.h#L53-L57 10:46 < MarcoFalke> So array conversion implies implicit conversion? 10:46 < sipa> MarcoFalke: yeah, this matches the criterion in std::span, which makes me confident there aren't edge cases left 10:47 < sipa> if it's possible to treat an array of T as an array of C (e.g. if you have an array of ints, you can treat it as an array of const ints), then converting a Span of C to a Span of T is also possible 10:47 < willcl_ark> well the code comment-given example of converting to const seems reasonable, but I'm not sure about converting to another child class 10:48 -!- clarify [~clarify@178.162.222.42.adsl.inet-telecom.org] has left #bitcoin-core-pr-reviews [] 10:48 < sipa> willcl_ark: less useful, but i believe it also permits adding a volatile qualifier 10:49 < jnewbery> The commit log confused me: "This prevents constructing a Span given two pointers into an array of B (where B is a subclass of A), at least without explicit cast to pointers to A." I think it is possible to construct Span (as long as those pointers to arrays are implicitly convertible) 10:49 < sipa> so if you have an array "B arr[5];" 10:50 < sipa> then Span(arr, arr+5) should not work 10:50 < jnewbery> or is it literally just conversions between cv-qualifiers? 10:50 < sipa> because if you have a function that accepts an array of A, you can't pass it an array of B 10:50 < sipa> jnewbery: i haven't come across other useful examples, to be honest 10:51 < MarcoFalke> Would "A arr[5]; Span(arr, arr+5);" compile? 10:51 < sipa> MarcoFalke: no 10:51 < sipa> but B arr[5]; Span((A*)arr, (A*)(arr+5)) will 10:51 < jnewbery> ah. "Given two pointers into an array". I think I get it now 10:52 < sipa> if you do the pointer casting yourself you bypass the protection (and get garbage) 10:52 < MarcoFalke> oh. I guess it might be useful when we want to (e.g.) pass a span of validation interfaces 10:52 < sipa> this is an interesting question, why std::span uses 'convertible array' as criterion instead of 'add cv qualifiers'. i don't know the answer 10:53 < sipa> MarcoFalke: i'm not sure that'd work 10:53 < MarcoFalke> Span{(CValidationInterface*) wallet_array, (CValidationInterface*) (wallet_array+5)}; 10:53 < fjahr> sorry, was the answer to the risks that the elements could have a different size or was there something else? i think i misunderstood the question that there was something else 10:53 < sipa> fjahr: indeed 10:53 < fjahr> tyt 10:53 < fjahr> ty 10:54 < sipa> Why is MakeSpan useful? Can’t it be replaced with just invoking the Span::Span constructor? 10:54 -!- jb55 [~jb55@gateway/tor-sasl/jb55] has quit [Quit: jb55] 10:55 < sipa> (and as we're running out of time, a hint: why does C++20 not have an equivalent std::make_span) 10:55 < sipa> i guess that's more an extra question than a hint, ignore me! 10:56 < petterhs> 0, 10:56 < michaelfolkson> It constructs a Span of the right type automatically 10:56 < sipa> yep 10:56 < MarcoFalke> but why can't the constructor do that? 10:56 < willcl_ark> does ::Span not do that? 10:57 < sipa> not in C++11 10:57 < michaelfolkson> No idea why not in C++20. It isn't, I checked 10:57 < sipa> you cannot construct a data type without specifying its template parameters 10:57 < sipa> if you try Span(std::vector{1,2,3}) it will complain about missing template 10:58 < sipa> however, since C++17, there exists something called deduction guides 10:58 < sipa> which are rules that a type can specify to figure out the template argument based on a constructor's arguments 10:59 < jnewbery> https://en.cppreference.com/w/cpp/language/class_template_argument_deduction 10:59 < sipa> so std::span, which is in C++20, uses those to avoid the need for a make_span 10:59 < sipa> and there you can just replace all uses of MakeSpan with calling the constructor directly 10:59 < sipa> without specifying the template argument 10:59 < sipa> jnewbery: indeed 10:59 < MarcoFalke> so on master the verbose version Span(std::vector{1,2,3}) would compile? 10:59 < sipa> MarcoFalke: indeed 11:00 < sipa> MakeSpan is just convenience to avoid needing to specify the 11:00 < sipa> ok, any very last minute comments/insights/questions? 11:00 < willcl_ark> so it will make the scripted diff when we use std::span smaller :) 11:01 < sipa> yeah 11:01 < sipa> nothing? 11:01 < jnewbery> that was really informative. I learned a lot 11:01 < sipa> ok! 11:02 < sipa> thank you all for attending 11:02 < jnewbery> Thanks for hosting, sipa! 11:02 < fjahr> sipa: thanks! 11:02 < troygiorshev> thx sipa! 11:02 < raj_> thanks sipa. Lot to digest.. 11:02 < felixweis> thanks sipa! 11:02 < andrewtoth> thanks sipa! 11:02 < michaelfolkson> Yeah thanks sipa, that was great 11:02 < emzy> Thanks sipa! 11:02 < MarcoFalke> thx. TIL about CTAD 11:02 < gimballock> (y) thanks 11:02 -!- jules69 [4486dbb5@pool-68-134-219-181.bltmmd.fios.verizon.net] has quit [Remote host closed the connection] 11:02 -!- gimballock [b8a4b1b9@d-184-164-177-185.fl.cpe.atlanticbb.net] has quit [Remote host closed the connection] 11:03 < willcl_ark> thanks Sipa! 11:03 < jnewbery> #endmeeting 11:03 < instagibbs> thanks sipa! 11:03 -!- petterhs [51a74f5f@95.81-167-79.customer.lyse.net] has quit [Remote host closed the connection] 11:05 < jnewbery> next week's meeting will be on PR18991: Cache responses to GETADDR to prevent topology leaks, hosted by gleb naumenko 11:06 < jnewbery> Notes and questions will be up soon in the normal place: https://bitcoincore.reviews/18991.html 11:08 < sipa> excellent 11:20 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 11:22 < jnewbery> today's meeting log: https://bitcoincore.reviews/18468.html 11:25 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 256 seconds] 11:40 -!- TheRec_ [~toto@drupal.org/user/146860/view] has quit [Ping timeout: 260 seconds] 11:56 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has quit [Ping timeout: 260 seconds] 11:57 -!- troygiorshev [~troygiors@CPEdcef09a0ed55-CM0c473d74be00.cpe.net.cable.rogers.com] has joined #bitcoin-core-pr-reviews 12:13 -!- Talkless [~Talkless@hst-227-49.splius.lt] has quit [Quit: Konversation terminated!] 12:14 -!- dongcarl [~dongcarl@unaffiliated/dongcarl] has joined #bitcoin-core-pr-reviews 12:39 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-eqdnsxwkcwlcrcwi] has quit [Ping timeout: 260 seconds] 12:41 -!- tryphe_ is now known as tryphe 12:43 -!- salvatoshi [~salvatosh@2001:b07:644b:1d18:d9ba:58cc:b4b:b7b3] has quit [Quit: Leaving] 12:51 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-mxvuiultneutpxko] has joined #bitcoin-core-pr-reviews 13:02 -!- figs [2f293fee@047-041-063-238.res.spectrum.com] has quit [Remote host closed the connection] 13:22 -!- thomasb06 [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has left #bitcoin-core-pr-reviews ["ERC (IRC client for Emacs 26.3)"] 13:27 -!- ecurrencyhodler [68ac287f@cpe-104-172-40-127.socal.res.rr.com] has quit [Remote host closed the connection] 13:28 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-mxvuiultneutpxko] has quit [Ping timeout: 260 seconds] 13:29 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has quit [Read error: Connection reset by peer] 13:29 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has joined #bitcoin-core-pr-reviews 13:53 -!- thomasb0` [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has joined #bitcoin-core-pr-reviews 13:53 -!- thomasb0` [~user@eth-west-pareq2-46-193-0-224.wb.wifirst.net] has left #bitcoin-core-pr-reviews [] 14:03 -!- meshcollider [meshcollid@gateway/shell/ircnow/x-jfxfdhpppihkokhb] has joined #bitcoin-core-pr-reviews 14:06 -!- shesek [~shesek@unaffiliated/shesek] has left #bitcoin-core-pr-reviews ["Leaving"] 14:16 -!- slivera [~slivera@103.231.88.10] has joined #bitcoin-core-pr-reviews 15:22 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 15:26 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 240 seconds] 15:55 -!- yiannix [~yiannix@2a01:4b00:850f:4500:3cfd:2424:d8bb:4ad2] has quit [Ping timeout: 272 seconds] 17:41 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 17:45 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 260 seconds] 17:48 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 17:56 -!- tryphe_ [~tryphe@unaffiliated/tryphe] has joined #bitcoin-core-pr-reviews 17:56 -!- tryphe [~tryphe@unaffiliated/tryphe] has quit [Ping timeout: 256 seconds] 18:00 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 240 seconds] 19:20 -!- vindard [~vindard@190.83.165.233] has quit [Remote host closed the connection] 20:37 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 20:40 -!- vasild_ [~vd@gateway/tor-sasl/vasild] has joined #bitcoin-core-pr-reviews 20:44 -!- vasild [~vd@gateway/tor-sasl/vasild] has quit [Ping timeout: 240 seconds] 20:44 -!- vasild_ is now known as vasild 21:34 -!- hashbasher [~hashbashe@bras-base-shbtpq2113w-grc-02-70-55-131-122.dsl.bell.ca] has quit [Quit: Lost terminal] 21:46 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has quit [Read error: Connection reset by peer] 21:47 -!- evanlinjin [~evanlinji@2404:4408:43ff:bb00:25ff:8619:7d6f:2d27] has joined #bitcoin-core-pr-reviews 22:00 -!- tryphe_ is now known as tryphe 22:03 -!- S3RK [~s3rk@47.246.66.115] has quit [Remote host closed the connection] 22:04 -!- S3RK [~s3rk@47.246.66.115] has joined #bitcoin-core-pr-reviews 22:08 -!- S3RK [~s3rk@47.246.66.115] has quit [Ping timeout: 265 seconds] 22:32 -!- andrewtoth [~andrewtot@gateway/tor-sasl/andrewtoth] has quit [Quit: Leaving]