Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove operator bool() from array_view implementation #140

Closed
neilmacintosh opened this issue Oct 12, 2015 · 32 comments
Closed

Remove operator bool() from array_view implementation #140

neilmacintosh opened this issue Oct 12, 2015 · 32 comments
Assignees
Labels

Comments

@neilmacintosh
Copy link
Collaborator

The operator encourages a usage pattern that is unclear about what property of the array_view is being tested.

@gdr-at-ms
Copy link
Member

Agree with the removal.
The use pattern was something like

if (auto view = grab_some_view_onto(v))
      do_some_work_with(view);

@Horstling
Copy link

I agree, operator bool() is ambiguous here.

We can check for a zero-length range with size() == 0, but what is the recommended way to check for a real empty range (a range pointing to nothing) then?

@neilmacintosh
Copy link
Collaborator Author

.data() == nullptr

@ilyapopov
Copy link

Is it a design decision to omit empty() member in the span class?

@neilmacintosh
Copy link
Collaborator Author

@ilyapopov No, just an implementation omission for the moment.

.empty() will be added for consistency with types like vector and array as way of checking if length() == 0.

@OlafvdSpek
Copy link
Contributor

I'd love to see operator bool for ranges. :p

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2013/n3509.htm

@hpesoj
Copy link

hpesoj commented Nov 20, 2015

Consider what you would expect this code to do:

#include <gsl.h>
#include <iostream>

int main()
{
    int arr[] = { 1, 2, 3 };

    gsl::span<int> a = {};
    gsl::span<int> b = { arr, arr };

    if (a == b && !a == !!b)
    {
        std::cout << "hmm...\n";
    }
}

I would suggest that, if it is kept, operator bool() should reflect some aspect of the value of the span. The .data() pointer is not part of the value; it is an implementation detail. This is reflected by the fact that operator==() can return true even if .data() returns different values.

@neilmacintosh
Copy link
Collaborator Author

@hpesoj operator bool() will not be kept.

@OlafvdSpek
Copy link
Contributor

Damn, why not? I love operator book.
On Nov 20, 2015 16:20, "Neil MacIntosh" notifications@github.com wrote:

@hpesoj https://github.com/hpesoj operator bool() will not be kept.


Reply to this email directly or view it on GitHub
#140 (comment).

@neilmacintosh
Copy link
Collaborator Author

@OlafvdSpek back to my original comment: operator bool() is ambiguous about whether it is testing empty() or data()==nullptr. By removing it, people can be clear about which property they wish to test.

@hpesoj
Copy link

hpesoj commented Nov 20, 2015

@neilmacintosh Okay!

@OlafvdSpek
Copy link
Contributor

@neilmacintosh I'm familiar with the argument but I think it's kinda silly. It just takes some time to get used to.. AFAIK containers and ranges don't really have a null-state, they do have an empty state.
The is_empty test comes up all the time while the is_null test comes up very, very rarely. So having operator bool for is_empty makes a lot of sense, at least for me.

@OlafvdSpek
Copy link
Contributor

Also note that you can't use empty() in a if (auto s = f()) context.

@Horstling
Copy link

@OlafvdSpek
if (auto s = f()) is a very good example for the ambiguity of operator bool(), because in this context I would expect it to test for the null-state, which apparently differs from your expectation.

@OlafvdSpek
Copy link
Contributor

Why would you expect that?
Containers and ranges don't really have a null-state.

@Horstling
Copy link

Because I'm used to it from checking pointers. So in my mind I read if (auto s = f()) as "if the return from f is not null" (which is true for every pointer-like type). It`s just unusual to check something for emptiness with a condition like this.

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

@Horstling In #202 I just made an extensive argument against the concept of a "null array". If operator bool() were to exist, it absolutely should not report whether .data() is null. This is an implementation detail, not an aspect of the value of the array.

@OlafvdSpek
Copy link
Contributor

@Horstling Numbers and iostreams can be checked like that too.. so it seems there's a problem with your thinking. ;)

@Horstling
Copy link

@hpesoj
Isn't that the point of the discussion? I would expect operator bool() to check for a null state, you argue against it -> ambiguity. Things could be different if there was no null-state (and even then I don't think checking emptiness with operator bool() would be a good idea).

@OlafvdSpek
Zero numbers are way more NULL than empty. And I don't think iostreams is the best role model for interface design ;)

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

@OlafvdSpek A lot of people (me included) consider implicit int to bool conversion to be a mistake in the language, or at the very least dubious behaviour. Some people (me not included) also consider implicit pointer to bool conversion in the same light, though I think the fact that nullptr is a genuine special case (it's the only pointer which cannot be dereferenced) makes it different from 0, which behaves exactly the same as any other number.

@OlafvdSpek
Copy link
Contributor

@hpesoj If statements are considered explicit conversions to bool.
If you don't want implicit conversions I'm sure there's a warning you could enable.

@OlafvdSpek
Copy link
Contributor

@Horstling Numbers don't have empty and null states, containers don't have null states and most ranges don't have null states.

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

@Horstling I'm happy with removal of operator bool() (it's in line with the current standard library container types). If the standards committee were to add operator bool() as a synonym for .empty() to all containers, I would also be happy. I agree there is ambiguity (e.g. you could expect operator bool() to tell you whether the array contains an even number of elements) but since .empty() is the only function which returns a bool (correct me if I'm mistaken) on standard library containers, I don't see too much confusion arising.

However, there is no ambiguity with regards to the "null" status of the array. Arrays cannot be null, and the belief that they can stems from a confusion about what an array is, which most likely stems from the way C/C++98 represents both optional values and arrays using pointers, and the associated bad practice of representing an optional value and an array using a single pointer.

@Horstling
Copy link

@OlafvdSpek
Numbers have a "null-ish" state for me, so it makes sense to me to check them with if(x). Containers don't have a null state, and in general you can't check them with if(x). So ranges would be quite special if they would allow to check for emptiness with if(x).

But like I said, the whole discussion is the best point for not offering a bool conversion: There are very different opinions about what this conversion should do, so whatever it would do, some users would be surprised. Which is bad.

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

@OlafvdSpek Okay, then I meant any conversion to bool (I forgot about the new explicit conversion operators). I do enable warnings to that effect. I was just pointing out that many people wouldn't accept int to bool conversion as a compelling argument. I am on your side though.

@OlafvdSpek
Copy link
Contributor

@Horstling Talking about a null-sh state is confusing and suggestive. Question is, does a type have a natural mapping to bool? Pointers do, IMO numbers do too and IMO containers / ranges do as well.

Let me ask, how often do you use is_null (for ranges)? What ranges are you currently using?

@OlafvdSpek
Copy link
Contributor

Note that for string_view there were requests to guarantee data() wouldn't return null, effectively removing the null-state.

@Horstling
Copy link

Removing the null-state and having a operator bool() are two different topics.

I agree that the null-state is a relict from good ol' C arrays, and it might be a good idea to get this sorted out.

But even if you remove the null-state from ranges, a operator bool() that checks for emptiness would break consistency, simply because no one else is using operator bool() for this, at least in the std world. And when I see something like if (auto x = f()), I will always see a check for null. So only if I know that x is in fact a range and if I know that ranges behave differently than everyone else I can "overrule" my first intution.

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

If anyone isn't convinced that a "null span/array" isn't a thing, run this code:

#include <gsl.h>
#include <iostream>

int main()
{
    int value;

    gsl::span<int> null_array = { nullptr, nullptr };
    gsl::span<int> empty_array = { &value, &value };

    if (null_array == empty_array)
    {
        std::cout << "\"null\" spans are indistinguishable from empty spans\n";
    }
}

@OlafvdSpek @Horstling I have realised that my rationale for disliking int to bool conversion can be applied to containers/ranges/arrays. The main practical reason I despise the idea of "null" meaning "empty" wrt C-style arrays (this is different from "null" being distinct from "empty") is that it means I can't treat all arrays uniformly. For example, just as I can increment 0 like any other number, so can I iterate over empty arrays just like any other array. "null" empty arrays require a conditional check before use, which is why they are evil.

Therefore, I probably have to oppose operator bool() on span if I don't want to be a hypocrite :P . Thinking about it, perhaps rule should be this:

Only implement operator bool() to report whether an object is in a valid/non-null state. If an object has no such state, do not implement operator bool().

Using this criterion, numbers and ranges should not implement operator bool(), but pointers and IOstream classes may. Works for me :-)

@OlafvdSpek
Copy link
Contributor

Why would your null_array require special care? A (for) loop over it works just fine.
Null spans are NOT identical (indistinguishable) to non-null spans but they ARE equal. ;)

@hpesoj
Copy link

hpesoj commented Nov 21, 2015

@OlafvdSpek It doesn't require special care, because null_array is an empty array (i.e. it has zero size). I was referring to the use of "null" C-style arrays to indicate an empty array, whether or not the size is zero, or even present. For example:

void foo(int* data, size_t size) {
    if (data) // it's possible that data == nullptr and size != 0
        for (size_t i = 0; i < size; ++i) { … }
    }
}

But this isn't even what we are discussing. It was just an example of how arrays could have valid/invalid states, and why this would be evil.

And I suppose it depends on what you mean by "indistinguishable". But yeah, my point was that if operator==() doesn't distinguish "null" spans from empty spans, then why should any sane programmer? In my opinion, checking whether .data() is null is similar to inspecting the address of the span itself. The only difference is that it's slightly easier to control the value of .data() for one's nefarious ends (muah ha ha ha ha!)

@neilmacintosh
Copy link
Collaborator Author

@Horstling @hpesoj This conversation appears to have veered a little off-topic. I have now removed operator bool() in PR #215 in line with the GSL design for span, so I'm going to close out this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants