You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Alan Conway <ac...@redhat.com> on 2016/06/07 21:23:14 UTC
Review Request 48362: PROTON-1221: c++ container::schedule() support.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48362/
-----------------------------------------------------------
Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross.
Repository: qpid-proton-git
Description
-------
PROTON-1221: c++ container::schedule() support.
Diffs
-----
examples/cpp/CMakeLists.txt 06ec1a49c00a98c3a602c9b6e76bf311345c397b
examples/cpp/README.dox 897b302da0c0c132093b5f9977d28ac274cfeea9
examples/cpp/mt/epoll_container.cpp feea3007a8b732a47177a50d6ff385f9d60f1a63
examples/cpp/scheduled_send.cpp PRE-CREATION
proton-c/bindings/cpp/include/proton/container.hpp 9665346cef187fccf59049ea4f5ce927dd46b60b
proton-c/bindings/cpp/include/proton/default_container.hpp f50869cb28b9d94784ab3329dcec43085eab4393
proton-c/bindings/cpp/src/container_impl.hpp 8fd64d00c4191c34ec3e6dd5da32c121f8ad2718
proton-c/bindings/cpp/src/container_impl.cpp ce6a2b2a097795d49f40e4a962e91db9222588f0
proton-c/bindings/cpp/src/test_dummy_container.hpp 730790140bea671796032b4daa95492ed1c17c25
Diff: https://reviews.apache.org/r/48362/diff/
Testing
-------
Thanks,
Alan Conway
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Alan Conway <ac...@redhat.com>.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > You need to compile this code with C++03 and fix up the places that end up needing a feature macro to avoid breakage.
>
> Alan Conway wrote:
> Yep, known limitation. I need to add C++03 callback support. This is a first cut in C++11 without worrying about that. All your comments will apply to the next cut.
>
> Quetion: How to organize examples for features that are used differntly in C++03 and C++11?
>
> - separate c++11/c++03 examples?
> - conditionalized examples that work under both, with different example code chunks selected by #ifdef?
>
> Clean C++11 examples would be nice. They have to be defined against a specific standard: 11, 14 or 17. We could choose a base standard (11) and use conditional macros for features of 14/17 but I think we should stick to clean c++11 - these are proton examples not C++ tutorials, if we can't write them in C++11 we should rethink the API. That means separate examples (with some redundancy) are needed at least for features that can't be used the same way in C++03 (schedule, inject)
>
> "Universal" conditionalized examples based on 03, with fake_cpp11.hpp, #if PN_CPP_HAS_STD_FUNCTION etc. : easier for us to maintain, maybe easier for users transitioning from 03 to 11 (side-by-side code) but cluttered for users that are on 11. On the other hand very few of the examples actually *need* C++11 features, so if we do have users stuck on older compilers it's a bit obnoxious to make the base examples uncompilable when they could easily be portable. It really hinges on how many users are in that position.
>
> A hybrid of the above: conditionalized examples where C++11 and 03 are nearly the same (basically what we have now) and separate examples where they diverge.
>
> I'm not sure, thoughts?
>
> Andrew Stitcher wrote:
> We've already decided this:
>
> The main examples will be C++11 with a separate C++03 directory with the basic examples (about 4 I think, I don't remember exactly which).
> (I think c++11 is sufficient although 14 would bring the convenience of make_unique<>)
>
> We decided that mt is C++11 only, I don't think we decided this for function injection.
>
> [Write this down, you seem to keep on forgetting we made this decision!]
Thanks, it is on the list of things I keep forgetting but I can't remember where I left it.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 44
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line44>
> >
> > Yep that's a good idea, lets add double constructors to duration (just not as pzrt of this change)
>
> Alan Conway wrote:
> Agreed, will restrain myself :) I think we need double `op*` rather than constructors to keep the units unambiguous: `duration d(2.0*duration::SECOND)`. A double ctor will still have you doing `duration d (2.0*duration::SECOND.milliseconds())`
>
> Andrew Stitcher wrote:
> That would be completely unambiguous, but I think that double seconds is pretty unambiguous tbh. `duration d(2.2); // duration of 2.2s`. Having the operator*(double, duration) operator*(duration, double) is still a good idea.
I considered that, but I think it is error-prone if duration(1) and duration(1.0) mean different things. Another (less explicit) solution is to have `const int64_t duration::SECOND` instead of the constants being `duration`: then all the right conversions happen implicitly with the normal C++ arithmetic promotion and conversion rules, no need for extra ctor or ops. Explicit is probably better though.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/container.hpp, line 201
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410590#file1410590line201>
> >
> > Need to add a new feature flag I'll need to look up the actual name, but PN_CPP_HAS_STD_FUNCTION will do for now.
>
> Alan Conway wrote:
> See examples question above
>
> Andrew Stitcher wrote:
> No, this is in a shared header file you *must* put a feature macro in here. Whether the examples are 03 or 11 doesn't matter.
Agreed, over-pasting.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 89
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line89>
> >
> > Should the delay here take account of the actual now time?
> >
> > In other words, are we trying for a fixed frequency or an "at least" delay?
>
> Alan Conway wrote:
> For purposes of showing how to use schedule() I don't think it matters - I'll add a comment to clarify this this example is doing a delay *between* sends. Making it fixed frequency would allow us to show the use of now(), so we could do that too.
Got another vote for fixed frequency so I'll update.
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48362/#review136551
-----------------------------------------------------------
On June 7, 2016, 9:23 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48362/
> -----------------------------------------------------------
>
> (Updated June 7, 2016, 9:23 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-1221: c++ container::schedule() support.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 06ec1a49c00a98c3a602c9b6e76bf311345c397b
> examples/cpp/README.dox 897b302da0c0c132093b5f9977d28ac274cfeea9
> examples/cpp/mt/epoll_container.cpp feea3007a8b732a47177a50d6ff385f9d60f1a63
> examples/cpp/scheduled_send.cpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 9665346cef187fccf59049ea4f5ce927dd46b60b
> proton-c/bindings/cpp/include/proton/default_container.hpp f50869cb28b9d94784ab3329dcec43085eab4393
> proton-c/bindings/cpp/src/container_impl.hpp 8fd64d00c4191c34ec3e6dd5da32c121f8ad2718
> proton-c/bindings/cpp/src/container_impl.cpp ce6a2b2a097795d49f40e4a962e91db9222588f0
> proton-c/bindings/cpp/src/test_dummy_container.hpp 730790140bea671796032b4daa95492ed1c17c25
>
> Diff: https://reviews.apache.org/r/48362/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Alan Conway <ac...@redhat.com>.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > You need to compile this code with C++03 and fix up the places that end up needing a feature macro to avoid breakage.
Yep, known limitation. I need to add C++03 callback support. This is a first cut in C++11 without worrying about that. All your comments will apply to the next cut.
Quetion: How to organize examples for features that are used differntly in C++03 and C++11?
- separate c++11/c++03 examples?
- conditionalized examples that work under both, with different example code chunks selected by #ifdef?
Clean C++11 examples would be nice. They have to be defined against a specific standard: 11, 14 or 17. We could choose a base standard (11) and use conditional macros for features of 14/17 but I think we should stick to clean c++11 - these are proton examples not C++ tutorials, if we can't write them in C++11 we should rethink the API. That means separate examples (with some redundancy) are needed at least for features that can't be used the same way in C++03 (schedule, inject)
"Universal" conditionalized examples based on 03, with fake_cpp11.hpp, #if PN_CPP_HAS_STD_FUNCTION etc. : easier for us to maintain, maybe easier for users transitioning from 03 to 11 (side-by-side code) but cluttered for users that are on 11. On the other hand very few of the examples actually *need* C++11 features, so if we do have users stuck on older compilers it's a bit obnoxious to make the base examples uncompilable when they could easily be portable. It really hinges on how many users are in that position.
A hybrid of the above: conditionalized examples where C++11 and 03 are nearly the same (basically what we have now) and separate examples where they diverge.
I'm not sure, thoughts?
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 29
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line29>
> >
> > This is C++11 only don't need "fake_cpp11.hpp", and can just use "override" directly.
See examples question above
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 54
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line54>
> >
> > Should be able to combine into a single line with no loss of readability (IMO):
> > One common style seems to be -
> >
> > ```
> > c.schedule(timeout, [this]() {
> > this->cancel();
> > });
> > ```
See examples question above.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 89
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line89>
> >
> > Should the delay here take account of the actual now time?
> >
> > In other words, are we trying for a fixed frequency or an "at least" delay?
For purposes of showing how to use schedule() I don't think it matters - I'll add a comment to clarify this this example is doing a delay *between* sends. Making it fixed frequency would allow us to show the use of now(), so we could do that too.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/container.hpp, line 201
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410590#file1410590line201>
> >
> > Need to add a new feature flag I'll need to look up the actual name, but PN_CPP_HAS_STD_FUNCTION will do for now.
See examples question above
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 44
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line44>
> >
> > Yep that's a good idea, lets add double constructors to duration (just not as pzrt of this change)
Agreed, will restrain myself :) I think we need double `op*` rather than constructors to keep the units unambiguous: `duration d(2.0*duration::SECOND)`. A double ctor will still have you doing `duration d (2.0*duration::SECOND.milliseconds())`
- Alan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48362/#review136551
-----------------------------------------------------------
On June 7, 2016, 9:23 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48362/
> -----------------------------------------------------------
>
> (Updated June 7, 2016, 9:23 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-1221: c++ container::schedule() support.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 06ec1a49c00a98c3a602c9b6e76bf311345c397b
> examples/cpp/README.dox 897b302da0c0c132093b5f9977d28ac274cfeea9
> examples/cpp/mt/epoll_container.cpp feea3007a8b732a47177a50d6ff385f9d60f1a63
> examples/cpp/scheduled_send.cpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 9665346cef187fccf59049ea4f5ce927dd46b60b
> proton-c/bindings/cpp/include/proton/default_container.hpp f50869cb28b9d94784ab3329dcec43085eab4393
> proton-c/bindings/cpp/src/container_impl.hpp 8fd64d00c4191c34ec3e6dd5da32c121f8ad2718
> proton-c/bindings/cpp/src/container_impl.cpp ce6a2b2a097795d49f40e4a962e91db9222588f0
> proton-c/bindings/cpp/src/test_dummy_container.hpp 730790140bea671796032b4daa95492ed1c17c25
>
> Diff: https://reviews.apache.org/r/48362/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Andrew Stitcher <as...@apache.org>.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > You need to compile this code with C++03 and fix up the places that end up needing a feature macro to avoid breakage.
>
> Alan Conway wrote:
> Yep, known limitation. I need to add C++03 callback support. This is a first cut in C++11 without worrying about that. All your comments will apply to the next cut.
>
> Quetion: How to organize examples for features that are used differntly in C++03 and C++11?
>
> - separate c++11/c++03 examples?
> - conditionalized examples that work under both, with different example code chunks selected by #ifdef?
>
> Clean C++11 examples would be nice. They have to be defined against a specific standard: 11, 14 or 17. We could choose a base standard (11) and use conditional macros for features of 14/17 but I think we should stick to clean c++11 - these are proton examples not C++ tutorials, if we can't write them in C++11 we should rethink the API. That means separate examples (with some redundancy) are needed at least for features that can't be used the same way in C++03 (schedule, inject)
>
> "Universal" conditionalized examples based on 03, with fake_cpp11.hpp, #if PN_CPP_HAS_STD_FUNCTION etc. : easier for us to maintain, maybe easier for users transitioning from 03 to 11 (side-by-side code) but cluttered for users that are on 11. On the other hand very few of the examples actually *need* C++11 features, so if we do have users stuck on older compilers it's a bit obnoxious to make the base examples uncompilable when they could easily be portable. It really hinges on how many users are in that position.
>
> A hybrid of the above: conditionalized examples where C++11 and 03 are nearly the same (basically what we have now) and separate examples where they diverge.
>
> I'm not sure, thoughts?
We've already decided this:
The main examples will be C++11 with a separate C++03 directory with the basic examples (about 4 I think, I don't remember exactly which).
(I think c++11 is sufficient although 14 would bring the convenience of make_unique<>)
We decided that mt is C++11 only, I don't think we decided this for function injection.
[Write this down, you seem to keep on forgetting we made this decision!]
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > examples/cpp/scheduled_send.cpp, line 44
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410589#file1410589line44>
> >
> > Yep that's a good idea, lets add double constructors to duration (just not as pzrt of this change)
>
> Alan Conway wrote:
> Agreed, will restrain myself :) I think we need double `op*` rather than constructors to keep the units unambiguous: `duration d(2.0*duration::SECOND)`. A double ctor will still have you doing `duration d (2.0*duration::SECOND.milliseconds())`
That would be completely unambiguous, but I think that double seconds is pretty unambiguous tbh. `duration d(2.2); // duration of 2.2s`. Having the operator*(double, duration) operator*(duration, double) is still a good idea.
> On June 7, 2016, 9:40 p.m., Andrew Stitcher wrote:
> > proton-c/bindings/cpp/include/proton/container.hpp, line 201
> > <https://reviews.apache.org/r/48362/diff/1/?file=1410590#file1410590line201>
> >
> > Need to add a new feature flag I'll need to look up the actual name, but PN_CPP_HAS_STD_FUNCTION will do for now.
>
> Alan Conway wrote:
> See examples question above
No, this is in a shared header file you *must* put a feature macro in here. Whether the examples are 03 or 11 doesn't matter.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48362/#review136551
-----------------------------------------------------------
On June 7, 2016, 9:23 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48362/
> -----------------------------------------------------------
>
> (Updated June 7, 2016, 9:23 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-1221: c++ container::schedule() support.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 06ec1a49c00a98c3a602c9b6e76bf311345c397b
> examples/cpp/README.dox 897b302da0c0c132093b5f9977d28ac274cfeea9
> examples/cpp/mt/epoll_container.cpp feea3007a8b732a47177a50d6ff385f9d60f1a63
> examples/cpp/scheduled_send.cpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 9665346cef187fccf59049ea4f5ce927dd46b60b
> proton-c/bindings/cpp/include/proton/default_container.hpp f50869cb28b9d94784ab3329dcec43085eab4393
> proton-c/bindings/cpp/src/container_impl.hpp 8fd64d00c4191c34ec3e6dd5da32c121f8ad2718
> proton-c/bindings/cpp/src/container_impl.cpp ce6a2b2a097795d49f40e4a962e91db9222588f0
> proton-c/bindings/cpp/src/test_dummy_container.hpp 730790140bea671796032b4daa95492ed1c17c25
>
> Diff: https://reviews.apache.org/r/48362/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Alan Conway <ac...@redhat.com>.
On Tue, 2016-06-07 at 20:15 -0400, Michael Goulish wrote:
>
>
> >
> > ����
> > ����In other words, are we trying for a fixed frequency or an "at
> > least"
> > ����delay?
> >
> What I need this feature for is to be able to send messages at a
> fixed frequency.
OK that's 2 votes, I'll redo the example for fixed frequency :)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Michael Goulish <mg...@redhat.com>.
>
> In other words, are we trying for a fixed frequency or an "at least"
> delay?
>
What I need this feature for is to be able to send messages at a fixed frequency.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org
Re: Review Request 48362: PROTON-1221: c++ container::schedule()
support.
Posted by Andrew Stitcher <as...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/48362/#review136551
-----------------------------------------------------------
You need to compile this code with C++03 and fix up the places that end up needing a feature macro to avoid breakage.
examples/cpp/scheduled_send.cpp (line 29)
<https://reviews.apache.org/r/48362/#comment201622>
This is C++11 only don't need "fake_cpp11.hpp", and can just use "override" directly.
examples/cpp/scheduled_send.cpp (line 44)
<https://reviews.apache.org/r/48362/#comment201623>
Yep that's a good idea, lets add double constructors to duration (just not as pzrt of this change)
examples/cpp/scheduled_send.cpp (line 54)
<https://reviews.apache.org/r/48362/#comment201627>
Should be able to combine into a single line with no loss of readability (IMO):
One common style seems to be -
```
c.schedule(timeout, [this]() {
this->cancel();
});
```
examples/cpp/scheduled_send.cpp (line 89)
<https://reviews.apache.org/r/48362/#comment201630>
Should the delay here take account of the actual now time?
In other words, are we trying for a fixed frequency or an "at least" delay?
proton-c/bindings/cpp/include/proton/container.hpp (line 201)
<https://reviews.apache.org/r/48362/#comment201631>
Need to add a new feature flag I'll need to look up the actual name, but PN_CPP_HAS_STD_FUNCTION will do for now.
proton-c/bindings/cpp/src/container_impl.cpp (line 170)
<https://reviews.apache.org/r/48362/#comment201632>
As above use a specific feature macro.
- Andrew Stitcher
On June 7, 2016, 9:23 p.m., Alan Conway wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48362/
> -----------------------------------------------------------
>
> (Updated June 7, 2016, 9:23 p.m.)
>
>
> Review request for qpid, Andrew Stitcher, Cliff Jansen, and Justin Ross.
>
>
> Repository: qpid-proton-git
>
>
> Description
> -------
>
> PROTON-1221: c++ container::schedule() support.
>
>
> Diffs
> -----
>
> examples/cpp/CMakeLists.txt 06ec1a49c00a98c3a602c9b6e76bf311345c397b
> examples/cpp/README.dox 897b302da0c0c132093b5f9977d28ac274cfeea9
> examples/cpp/mt/epoll_container.cpp feea3007a8b732a47177a50d6ff385f9d60f1a63
> examples/cpp/scheduled_send.cpp PRE-CREATION
> proton-c/bindings/cpp/include/proton/container.hpp 9665346cef187fccf59049ea4f5ce927dd46b60b
> proton-c/bindings/cpp/include/proton/default_container.hpp f50869cb28b9d94784ab3329dcec43085eab4393
> proton-c/bindings/cpp/src/container_impl.hpp 8fd64d00c4191c34ec3e6dd5da32c121f8ad2718
> proton-c/bindings/cpp/src/container_impl.cpp ce6a2b2a097795d49f40e4a962e91db9222588f0
> proton-c/bindings/cpp/src/test_dummy_container.hpp 730790140bea671796032b4daa95492ed1c17c25
>
> Diff: https://reviews.apache.org/r/48362/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Alan Conway
>
>