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

Fix is_buffer_sequence #403

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

karzhenkov
Copy link

@karzhenkov karzhenkov commented Jul 3, 2019

Almost completely reworked is_buffer_sequence implementation is intended to fix issues from #402 (and to satisfy the corresponding test). It is also more concise, since it does not introduce several helper declarations for each individual check. A few helper declarations of general use are placed in the separate header file.

This PR obsoletes #402, however, some comments still can be found there.

Here is "rebased" branch containing all the stuff of this PR presented as two independent changesets (I would prefer not to force-push yet again). The first one is the test from #402; this second contains proposed fixes.

Original logic behind the is_buffer_sequence template for all types except mutable_buffer and const_buffer is guessed as follows:

  • When decltype is available, the type X is considered as MutableBufferSequence if the result of the expression *begin_buffer_sequence(x) can be converted to mutable_buffer and end_buffer_sequence(x) returns non-void. Said functions in general case return the same as corresponding member functions, so the reqiurements above can be applied to expressions *x.begin() and x.end().
  • When decltype is unavailable or disabled, the type X is considered as MutableBufferSequence if it has the following: members named begin and end (not nessesary the functions); nested types or typedefs const_iterator and value_type. The latter type also has to be convertible to mutable_buffer.

Reqiurements for ConstBufferSequence are similar.

The logic described is implemented incorrectly; test cases and possible fixes are proposed in #402. This logic is also not perfect. Some obvious checks in #402 still fail.

The proposed variant implements the following set of requirements for MutableBufferSequence and ConstBufferSequence (with no dependency on the avaliability of decltype):

  1. Expression *begin_buffer_sequence(x) has to be convertible to the corresponding buffer type (mutable_buffer or const_buffer).
  2. Expression *end_buffer_sequence(x) has to be convertible to the corresponding buffer type.

Nested type const_iterator (and also iterator) will be checked indirectly through declarations of begin_buffer_sequence and end_buffer_sequence when necessary (it may be necessary only when decltype is unavailable).

@raJeev-M
Copy link

raJeev-M commented Jul 3, 2019

Have not looked at the implementation yet, will do so, but before that.

Original logic ..
When decltype is available...
When decltype is unavailable...

The proposed variant implements.....(with no dependency on the avaliability of `decltype' )

Are these statements not contradicting themselves? from the first two quote we can clearly see that there is no dependency (currently) on decltype.

@karzhenkov karzhenkov force-pushed the fix-is_buffer_sequence branch from d855e3d to fb247ee Compare July 3, 2019 11:43
@karzhenkov
Copy link
Author

Hi, @raJeev-M
There is currently a dependency on decltype. I tried to describe it in "original logic" part. Perhaps I did it not quite clear, however you can see the difference when run the test from #402 (very first commit 055c51f) with decltype enabled/disabled (I used `ASIO_DISABLE_DECLTYPE).

@karzhenkov karzhenkov marked this pull request as ready for review July 3, 2019 12:06
@raJeev-M
Copy link

raJeev-M commented Jul 3, 2019

Hi, @raJeev-M
There is currently a dependency on decltype. I tried to describe it in "original logic" part. Perhaps I did it not quite clear, however you can see the difference when run the test from #402 (very first commit 055c51f) with decltype enabled/disabled (I used `ASIO_DISABLE_DECLTYPE).

Still, I don't think I will use the word dependency , in is_buffer_sequence I see it says ASIO_HAS_DECLTYPE (otherwise use enable_if<>): which sounds like optional not dependent.

@karzhenkov
Copy link
Author

I agree, decltype is optional; the word "dependency" is probably inappropriate here.
The probem is that

  • the program behavior differs significantly when compiled with/without this option;
  • in both cases the program behavior may be incorrect.

@karzhenkov
Copy link
Author

is_dynamic_buffer templates are improved to more closely meet the documented DynamicBuffer requirements (and also to be a bit more readable).

template <typename T>
static result<sizeof(

is_same_as<std::size_t>(declval<T>().size()),
Copy link
Author

Choose a reason for hiding this comment

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

Perhaps we should make this check (and other similar checks) more strict:

Suggested change
is_same_as<std::size_t>(declval<T>().size()),
is_same_as<std::size_t>(declval<const T>().size()),

And also to relax the return type requirement:

Suggested change
is_same_as<std::size_t>(declval<T>().size()),
is_convertible_to<std::size_t>(declval<T>().size()),

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