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

Test for is_buffer_sequence: false positives #402

Closed

Conversation

karzhenkov
Copy link

Simple test cases indicate that the is_buffer_sequence template is implemented incorrectly:

  • struct A1 with single member is recognized as a MutableBufferSequence when compile with the decltype available;
  • struct B1 with inappropriate typedefs and no member functions is recognized as a MutableBufferSequence when the decltype is unavailable or disabled.

@karzhenkov karzhenkov marked this pull request as ready for review July 2, 2019 18:46
karzhenkov added a commit to karzhenkov/asio-primary that referenced this pull request Jul 3, 2019
karzhenkov added a commit to karzhenkov/asio-primary that referenced this pull request Jul 3, 2019
typename enable_if<!is_same<
decltype(asio::buffer_sequence_end(*t)),
void>::value>::type*);
void>::value>::type*))[2];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

sizeof(buffer_sequence_begin_helper<T>(0)) != 1 &&
sizeof(buffer_sequence_end_helper<T>(0)) != 1 &&
sizeof(buffer_sequence_begin_helper<T>(0, 0)) != 1 &&
sizeof(buffer_sequence_end_helper<T>(0, 0)) != 1 &&
Copy link

@raJeev-M raJeev-M Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the fallback becomes char[2] not char, so should it not be !=2

Copy link
Author

@karzhenkov karzhenkov Jul 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When called with single argument this is resolved to the fallback, since no default argument value in checker function declaration is specified. The fallback was char[2], the condition was true regardless of the type being tested.

The modified fallback for ASIO_HAS_DECLTYPE case is char, so != 1 is correct.

When ASIO_HAS_DECLTYPE is not defined, fallback is still char[2], but in this case checker function must be rejected for acceptable type; != 1 is correct again.

typename enable_if<!is_same<
decltype(asio::buffer_sequence_begin(*t)),
void>::value>::type*);
void>::value>::type*))[2];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this change?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fallback is now char when ASIO_HAS_DECLTYPE is defined (see above).
For checker function we need char[2]

@raJeev-M
Copy link

raJeev-M commented Jul 3, 2019

struct A1 with single member is recognized as a MutableBufferSequence when compile with the decltype available;

I am not sure why do you say it, Have looked at your test-code, here is my observation and correct me if am wrong

so you have the below stuff

struct A1
{
  mutable_buffer* begin();

  // no "value_type" type
  // no "const_iterator" type
  // no "end" member function
};

ASIO_CHECK(!is_mutable_buffer_sequence<A1>::value);

I believe is_mutable_buffer_sequence<A1> returns true.. and it bombs you out because there is no
end(), yet, still the buffer_sequence_end_helper is not falling-back?

Here is my experiment on asio::buffer_sequence_end_helper, looks like, things are working fine as expected: https://godbolt.org/z/Ch_jCq . Do you think I am miss anything?

@karzhenkov
Copy link
Author

When I add mutable_buffer* end(); to struct A1 in your sample, it still compile successfully. The second overload of helper (i. e. the checker) will always be rejected independently of declarations in struct A1. Condition will always be true.

To change this we can provide default value for its second argument (or add second argument when it is called from static_assert).

After such a change we need also invert condition, and write != 2, but this will break the logic for the case when ASIO_HAS_DECLTYPE is not defined.

So instead of != 2 we have to "swap" return types of the fallback and the checker.

@karzhenkov
Copy link
Author

karzhenkov commented Jul 3, 2019

When ASIO_HAS_DECLTYPE for acceptable type the checker must be fallback return type before change fallback return type after change
is defined accepted (but was rejected for any type) char[2] char
is not defined rejected char[2] char[2]

@karzhenkov
Copy link
Author

karzhenkov commented Jul 4, 2019

ASIO_CHECK(!is_mutable_buffer_sequence<A1>::value);

I believe is_mutable_buffer_sequence<A1> returns true.. and it bombs you out because there is no
end(), ...

I would interpret this situation a bit differently.

The test (055c51f) is written with assumption that A1 should not be considered as MutableBufferSequence because it has no end(). The test failed, i. e. the program wrongly accepted A1 as a valid MutableBufferSequence. This indicates an error in the is_buffer_sequence implemenation.

Or perhaps the assumption used in the test is wrong?

karzhenkov added a commit to karzhenkov/asio-primary that referenced this pull request Jul 6, 2019
@karzhenkov
Copy link
Author

Obsoleted by #403

@karzhenkov karzhenkov closed this Jul 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants