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

[WIP] Fix ArrayView bool conversion #43

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
1 participant
@mosra
Copy link
Owner

mosra commented Apr 2, 2018

The problem, in short:

int a[5];
Debug{} << bool(ArrayView<int>{a}); // prints true, as expected
Debug{} << bool(ArrayView<int>{a, 4}); // prints true
Debug{} << bool(ArrayView<int>{a, 0}); // prints true .. huh?

Debug{} << bool(ArrayView<int>{nullptr}); // prints false, as expected
Debug{} << bool(ArrayView<int>{nullptr, 0}); // prints false
Debug{} << bool(ArrayView<int>{nullptr, 4}); // prints false .. huh?

In other words, currently bool conversion is done from the data pointer, not from the size pointer. I think it should depend on the size rather than on the data, to make life easier when writing parsers and such. The following code will cycle forever, because input is always some non-null pointer, even though size eventually becomes zero and one has to pay extra attention and say !input.empty() or something:

ArrayView<const char> input; // some larger string

while(input) { // forever :(
    std::size_t bytesRead = ...;
    input = input.suffix(bytesRead);
}

That's not really intuitive, as e.g. std::istream also returns false when reaching EOF.


So, fixing the bool conversion like this (as partially done in the PR already) seems like a good idea (for me at least), but there's one problem -- the bool conversion operator is disabled on MSVC as it causes ambiguities for operator+, basically clashing with the pointer conversion operator, even though the bool conversion is explicit (huh?). So there are the following options:

  • Leave explicit operator bool() like it currently is (return _data;) and update the documentation to say it basically doesn't do what would one expect. That makes it consistent with MSVC, but practically useless and potentially dangerous if accidentally used for the wrong purpose.
  • FIx it, but since it's disabled on MSVC, have totally different behavior there, causing code to break in unexpected ways. Wrong solution.
  • Investigate and try to fix the ambiguity on MSVC. Might be impossible or might be possible only on MSVC 2017, leaving 2015 with the same dangerous inconsistency as above.
  • Add an explicit branch to the constructor which sets the data pointer to nullptr in case size is zero. That makes it consistent with Array behavior, but might break some code. (I know about at least one case in the OpenDDL parse where I'm treating zero-sized nullptr view and zero-sized non-nullptr view differently -- the first one indicating a parse error, the second indicating EOF.)

To ensure consistency, this applies also to the StaticArrayView, but there zero-sized views are problematic at the moment due to some non-ISO-standard zero-sized arrays being present in function signatures.

Opinions? Thank you :)

@mosra mosra added this to TODO in Containers via automation Apr 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.