Need help understanding parts of span.h

[ViralTaco]

Hi, in span.h you define a strange macro

bitcoin/src/span.h

Line 14 in 965e937

#define CONSTEXPR_IF_NOT_DEBUG

could someone please tell me the reasoning behind this?
It doesn't seem to be mentionned in the comments, maybe I overlooked it.

The reason I don't understand is that it, as far as I know, does not have to change anything whatsoever in terms of code generation.
Static initialization can happen without constexpr. (In fact it's a sneaky way to introduce bugs whenever you have a global std::map… It's a lot of "fun" but I digress).

Then we have this

bitcoin/src/span.h

Lines 116 to 125 in 965e937

	/** Construct a span from a begin and end pointer.
	*
	* This implements a subset of the iterator-based std::span constructor in C++20,
	* which is hard to implement without std::address_of.
	*/
	template <typename T, typename std::enable_if<std::is_convertible<T (*)[], C (*)[]>::value, int>::type = 0>
	CONSTEXPR_IF_NOT_DEBUG Span(T* begin, T* end) noexcept : m_data(begin), m_size(end - begin)
	{
	ASSERT_IF_DEBUG(end >= begin);
	}

Now I'm very confused.
Is this checking if an incomplete type is convertible to another incomplete type?
I don't know how that template could possibly do anything else. It's not checking if T** is implicitly convertible to C**, is it? I don't know what that does.

Why is it only constexpr when not in debug?
Why is it only doing this, rather important looking, assertion in debug?
Does it become valid to violate that assertion in prod?
Why? It seems odd and bug prone.

Thank you for your time.

[ViralTaco]

The more I look at that constructor the less sense it makes. When are you passing pointer at compile time?
Why does the check for end being at least at begin or after happen after we've 'set' m_size to a very large value?
Shouldn't this constructor throw in that case? Or at least, I don't know, somehow indicate that it has an invalid state?
Because using a span constructed with invalid pointers would result in invoking UB eg: if end() - 1 is dereferenced.
Where is that pointer? Not in a range that starts at begin(), that's all we know. I'm pretty sure that breaks the whole STL library without much of a warning.

EDIT: I've talked to other devs. So I understand a bit better. It's a narrow contract in std::span.
Makes sense so far. And this is a "drop-in" implementation, so the goal is for it to work the same with Span and std::span.
Which is great. The thing is: if it's UB now we can detect it and make sure we don't execute invalid code.
If it's UB with std::span we can't do anything.
If it's UB now, we do nothing about it, then it's UB in std::span it doesn't have to behave the same way anyway, or in any way for that matter.

TL;DR: can we please keep those assertion in the non "DEBUG" build? Because chances are until a user plays with it it's not gonna break, and I'd argue we want stuff to break in a manner that is evident to everyone involved.
It's a luxury we lose as soon as we use standard library feature.

[maflcko]

It's a luxury we lose as soon as we use standard library feature.

No, even with the standard library you can check for UB by using sanitizers (UBSan, ASan, MSan, valgrind, ...)

[ViralTaco]

It's a luxury we lose as soon as we use standard library feature.

No, even with the standard library you can check for UB by using sanitizers (UBSan, ASan, MSan, valgrind, ...)

Yes, that would work most of the time. It's not a wide contract, though. I was saying here we could make it an explicit requirement, with std::span we can't.
Please don't rely on any of those tools to detect ALL UB, though. They can miss some. They have.
Code can compile and run fine for years, and stop working one day. It's a lot harder to fix a bug than it is to prevent it, in my experience.

[sipa]

I'd prefer not to deviate from the std::span interface. Spans are used in some very tight loops, where performance matters. They're intended to be so cheap that they're not worse than just passing a begin pointer and length around all the time.

[maflcko]

Please don't rely on any of those tools to detect ALL UB, though. They can miss some. They have.

It was just a list of examples, not an exhaustive list. There is also a standard library debug option, which covers iterators: https://libcxx.llvm.org//DesignDocs/DebugMode.html#iterator-debugging-checks. So when we switch to std::span this debug option might or might not debug spans, too.

Also, the existing ASSERT_IF_DEBUG doesn't catch ALL UB either.

I was saying here we could make it an explicit requirement, with std::span we can't.

The requirement exists for both explicitly, but it is only possible (in the general case) to catch this with a runtime check. I don't think this is a (theoretical) reason to avoid switching to std::span, unless you can show a likely bug that is caught by this runtime check and won't be caught by any of our existing sanitizers while using std::span.

[ViralTaco]

I'd prefer not to deviate from the std::span interface. Spans are used in some very tight loops, where performance matters. They're intended to be so cheap that they're not worse than just passing a begin pointer and length around all the time.

I realized that, that's why I made a pull request just adding documentation. In this case it makes sense to avoid a redondant check (even if I have a personal preference for safety; in this case it has a real cost in terms of percieved complexity).

Also, the existing ASSERT_IF_DEBUG doesn't catch ALL UB either.
It doesn't catch any UB, technically. std::size_t is unsigned so if the number is negative it uses modulo arithmetic, iirc.

That's not the UB part. The UB part would be trying to use that span after it was created with that invalid state.
It's perfectly legal, and valid to create Span with begin coming after end. It's UB with std::span because std::span comes with a standard definition.

3. Constructs a span that is a view over the range [first, last); […]
   The behavior is undefined if [first, last) is not a valid range, […]

cf: https://en.cppreference.com/w/cpp/container/span/span

Which is why my proposed change is just documentation. In this case it's all that is needed to keep the same behavior as std::span whilst making it obvious.

I was confused by the "weird" macros. In hindsight it makes more sense.
Enough sense to conceide that the code doesn't need changing, but a comment wouldn't hurt. (In my case it would have saved a couple hours of confusion).

Someone had to point out to me "They're testing for the narrow contract in debug builds".
That's what is happening; It's not obvious when reading the code ASSERT_IF_DEBUG reads to me exactly like assert which already asserts only if NDEBUG isn't defined.

Still not sure about the CONSTEXPR_IF_NOT_DEBUG I'm assuming it just avoids Ill formed NDR
It could be named CONSTEXPR_IF_NO_ASSERT but that's needlessly nitpicky at this point.

[maflcko]

Ok, closing as this seems to be resolved. Let us know if you have any other questions.