You are viewing a plain text version of this content. The canonical link for it is here.
Posted to proton@qpid.apache.org by alanconway <gi...@git.apache.org> on 2015/06/03 19:37:49 UTC

[GitHub] qpid-proton pull request: C++ binding for proton.

GitHub user alanconway opened a pull request:

    https://github.com/apache/qpid-proton/pull/35

    C++ binding for proton.

    Cliff Jansen has been working on an event-driven C++ binding for proton, similar to the python binding.
    The work is not complete but I think it is almost ready to move to trunk for easier access.  Please see proton-c/bindings/cpp/README.md for  a TODO list. I'll be working on the TODO list this week and will move the binding to trunk next week if there are no objecctions. Any other comments are most welcome.


You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/apache/qpid-proton cjansen-cpp-client

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/qpid-proton/pull/35.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #35
    
----
commit ad7c9778f4ea33a6ca982ac4fea5178b0320b24d
Author: Cliff Jansen <cl...@apache.org>
Date:   2015-04-30T18:22:03Z

    PROTON-865: new cjansen-cpp-client branch for fledgling C++ client code using the event reactor

commit 648f7b36caa26e8078b92b50391d2e9f23e16080
Author: Cliff Jansen <cl...@apache.org>
Date:   2015-05-05T13:48:48Z

    PROTON-865: most of MessagingAdapter in place, SimpleSend/Recv

commit 3c0c90f74c053657d04cd7ba6f357f851c101999
Author: Cliff Jansen <cl...@apache.org>
Date:   2015-05-07T14:06:43Z

    PROTON-865: Message properties, Acking and Delivery

commit e8acc1e5f58975dc9d1d99d779f5e3c77289a4b1
Author: Cliff Jansen <cl...@apache.org>
Date:   2015-05-12T02:29:53Z

    PROTON-865: Added basic Terminus, Broker.cpp example

commit d56c5d7664334af3126e1509bef5791593a08c35
Author: Cliff Jansen <cl...@apache.org>
Date:   2015-05-20T14:54:03Z

    PROTON-865: Blocking sender functionality and handler per connection

commit 8074793b84a6402b01f3639e8cf8fed6b5ba54cc
Author: Alan Conway <ac...@redhat.com>
Date:   2015-06-01T22:03:36Z

    PROTON-865: Example and cmake cleanup, minor code cleanup.
    
    - Optionally enable C++ in top level cmake for examplesb. Still works without a C++ compiler.
    - Move cpp examples to the /examples dir, renamed for consistent exe naming conventions.
    - Integrate example build & auto-test with cmake
    - Improved exception handling
    - Simplified exception handling.
    - Removed nascent logging code, logging should be handled centrally in C library.

----


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31834639
  
    --- Diff: proton-c/bindings/cpp/README.md ---
    @@ -0,0 +1,24 @@
    +# C++ binding for proton.
    +
    +This is a C++ wrapper for the proton reactor API.
    --- End diff --
    
    It's not a "wrapper for the proton reactor" API it's a "C++ binding for the Proton API"


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31833264
  
    --- Diff: examples/cpp/example_test.py ---
    @@ -0,0 +1,105 @@
    +#
    --- End diff --
    
    Is this file being added by mistake? (It seems to be a python example not C++)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31842778
  
    --- Diff: proton-c/bindings/CMakeLists.txt ---
    @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND)
       set (DEFAULT_JAVASCRIPT ON)
     endif (EMSCRIPTEN_FOUND)
     
    +# Prerequisites for C++
    --- End diff --
    
    There is a foreach at the end of the file that sets up all the user-settable binding options. For each binding you must set a DEFAULT_FOO to be on or off depending if the requirements to build the binding are available, then the foreach sets up the BUILD_FOO option. CMAKE_CXX_COMPILER is set if a C++ compiler is available.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31844356
  
    --- Diff: CMakeLists.txt ---
    @@ -20,16 +20,15 @@ cmake_minimum_required (VERSION 2.6)
     
     project (Proton C)
     
    +# Enable C++ now for examples and bindings subdirectories, but make it optional.
    +enable_language(CXX OPTIONAL)
    +
    --- End diff --
    
    You are correct. I think that we could now move the block below somewhere else - it was only here to get the project command in the top level file.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109412800
  
    On Fri, 2015-06-05 at 11:13 -0700, Andrew Stitcher wrote:
    > Some overall comments (from a not yet thorough) read through:
    > I really don't like using set/get in a C++ API. It's not idiomatic 
    > C++ API style; it's not necessary; and it doesn't read as well (IMO) 
    > So instead of
    > Proton::transport::setXXXX('blah')/Proton::transport::getXXXX(), can
    > we please use
    > Proton::transport::XXXX('blah')/Proton::transport::XXXX().
    > We use idiomatic properties in Python we should use them in C++ too.
    
    100% agree, will fix. Those get/sets in Qpid have annoyed me for years.
    
    > I'm not sure that we really need to use the Pimpl pattern for this 
    > binding. The real need for a Pimpl is to insulate this API from 
    > changes to code that it depends on - in this case once the C API is 
    > stable then its own API stability requirements should ensure that 
    > nothing gets broken in the C++ API too. I may not have thought this 
    > through hard enough though [is that a sentence with too many 'ough's 
    > or what ;-) ]
    > This will save the indirection and extra resource use that Pimpl
    > brings, which would force 2 allocations for each of the structures.
    
    Agreed. Cliff also raised this question. He added some extra state to
    those classes to make things more user-friendly, but I think we should
    push such "helper" functionality to separate classes and keep a layer
    that is simply a C++ wrapper of a C pointer.
    
    > This strikes me as important because the C++ API can be as efficient 
    > as the C API if we do this work well, and it will be much easier to 
    > write and read code in good C++ than C. So I think that no one should 
    > need to do any application work in C even for applications that need 
    > the full efficiency of low level control of the library.
    
    Yup, agreed.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-172062911
  
    Out of date, latest is on master branch.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109423155
  
    There are a number of places in the API where the API has something like:
    `void setXXX(const char* bytes, size_t size);` this isn't very C++ like - in fact it adds very little value over the original API whilst still adding an extra layer wrapping it. If we are going to add a wrapper like this we should be making it more C++ like either using `void XXXX(const std::string&);` or `void XXXX(const std::vector<char>&);` (one or other or both depending on the context).
    
    This is equally true for getters as setters, although for these we might need to create some wrapping types to avoid copying if the value returned is a pointer into an existing value.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109383948
  
    Some overall comments (from a not yet thorough) read through:
    
    * I really don't like using set/get in a C++ API. It's not idiomatic C++ API style; it's not necessary; and it doesn't read as well (IMO) -
    
    So instead of `Proton::transport::setXXXX('blah')`/`Proton::transport::getXXXX()`, can we please use `Proton::transport::XXXX('blah')`/`Proton::transport::XXXX()`.
    
    We use idiomatic properties in Python we should use them in C++ too.
    
    * I'm not sure that we really need to use the Pimpl pattern for this binding. The real need for a Pimpl is to insulate this API from changes to code that it depends on - in this case once the C API is stable then its own API stability requirements should ensure that nothing gets broken in the C++ API too. I may not have thought this through hard enough though [is that a sentence with too many 'ough's or what ;-) ]
    
    This will save the indirection and extra resource use that Pimpl brings, which would force 2 allocations for each of the structures.
    
    This strikes me as important because the C++ API can be as efficient as the C API if we do this work well, and it will be much easier to write and read code in good C++ than C. So I think that no one should need to do any application work in C even for applications that need the full efficiency of low level control of the library.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31832198
  
    --- Diff: CMakeLists.txt ---
    @@ -20,16 +20,15 @@ cmake_minimum_required (VERSION 2.6)
     
     project (Proton C)
     
    +# Enable C++ now for examples and bindings subdirectories, but make it optional.
    +enable_language(CXX OPTIONAL)
    +
    --- End diff --
    
    The code block below already conditionally enables C++, the blocks should be merged


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31842586
  
    --- Diff: CMakeLists.txt ---
    @@ -20,16 +20,15 @@ cmake_minimum_required (VERSION 2.6)
     
     project (Proton C)
     
    +# Enable C++ now for examples and bindings subdirectories, but make it optional.
    +enable_language(CXX OPTIONAL)
    +
    --- End diff --
    
    enable_language(CXX OPTIONAL) enables cmake support for C++ if it is available. I had to move it up from proton-c because CMake requires you do this above any subdirs that will use it (now examples and proton-c). The next block just decides whether or not to use C++ for building proton. The C++ binding is default enabled if C++ is available (checks CXX_COMPILER), and can explicitly enabled/disabled with BUILD_CPP like all other bindings.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31844238
  
    --- Diff: proton-c/bindings/cpp/include/proton/cpp/Acceptor.h ---
    @@ -0,0 +1,50 @@
    +#ifndef PROTON_CPP_ACCEPTOR_H
    +#define PROTON_CPP_ACCEPTOR_H
    +
    +/*
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + *
    + */
    +#include "proton/cpp/ImportExport.h"
    +#include "proton/cpp/ProtonHandle.h"
    +#include "proton/reactor.h"
    +
    +struct pn_connection_t;
    +
    +namespace proton {
    +namespace reactor {
    +
    +class Acceptor : public ProtonHandle<pn_acceptor_t>
    +{
    +  public:
    +    PROTON_CPP_EXTERN Acceptor();
    +    PROTON_CPP_EXTERN Acceptor(pn_acceptor_t *);
    +    PROTON_CPP_EXTERN Acceptor(const Acceptor&);
    --- End diff --
    
    C++ is unbelievable. You have a default so you don't need to constantly re-type repetitive completely uninformative code, and now we have a special syntax so you can type something to show that you know you don't have to type it.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31834366
  
    --- Diff: proton-c/bindings/CMakeLists.txt ---
    @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND)
       set (DEFAULT_JAVASCRIPT ON)
     endif (EMSCRIPTEN_FOUND)
     
    +# Prerequisites for C++
    --- End diff --
    
    I don't understand what this block is doing


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109426860
  
    On Fri, 2015-06-05 at 13:05 -0700, Andrew Stitcher wrote:
    > There are a number of places in the API where the API has something 
    > like:
    > void setXXX(const char* bytes, size_t size); this isn't very C++ like 
    > - in fact it adds very little value over the original API whilst 
    > still adding an extra layer wrapping it. If we are going to add a 
    > wrapper like this we should be making it more C++ like either using 
    > void XXXX(const std::string&); or void XXXX(const 
    > std::vector<char>&); (one or other or both depending on the context).
    > This is equally true for getters as setters, although for these we 
    > might need to create some wrapping types to avoid copying if the 
    > value returned is a pointer into an existing value.
    
    Agreed. I would be inclined to do std::string in the C++ API and
    provide access to the underlying pn_foo_t* for edge cases.
    



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31842854
  
    --- Diff: proton-c/bindings/cpp/README.md ---
    @@ -0,0 +1,24 @@
    +# C++ binding for proton.
    +
    +This is a C++ wrapper for the proton reactor API.
    --- End diff --
    
    Agreed, fixed.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by gemmellr <gi...@git.apache.org>.
Github user gemmellr commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-172041707
  
    Can we close this old PR, doesnt seem like its being used anymore?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31844585
  
    --- Diff: proton-c/bindings/cpp/include/proton/cpp/Acceptor.h ---
    @@ -0,0 +1,50 @@
    +#ifndef PROTON_CPP_ACCEPTOR_H
    +#define PROTON_CPP_ACCEPTOR_H
    +
    +/*
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + *
    + */
    +#include "proton/cpp/ImportExport.h"
    +#include "proton/cpp/ProtonHandle.h"
    +#include "proton/reactor.h"
    +
    +struct pn_connection_t;
    +
    +namespace proton {
    +namespace reactor {
    +
    +class Acceptor : public ProtonHandle<pn_acceptor_t>
    +{
    +  public:
    +    PROTON_CPP_EXTERN Acceptor();
    +    PROTON_CPP_EXTERN Acceptor(pn_acceptor_t *);
    +    PROTON_CPP_EXTERN Acceptor(const Acceptor&);
    --- End diff --
    
    Not quite - that syntax tells the compiler to definitely generate the default implementation. It won't always otherwise, subject to irritating ules that might change as they are a little contraversial


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31843690
  
    --- Diff: proton-c/bindings/cpp/include/proton/cpp/Acceptor.h ---
    @@ -0,0 +1,50 @@
    +#ifndef PROTON_CPP_ACCEPTOR_H
    +#define PROTON_CPP_ACCEPTOR_H
    +
    +/*
    + *
    + * Licensed to the Apache Software Foundation (ASF) under one
    + * or more contributor license agreements.  See the NOTICE file
    + * distributed with this work for additional information
    + * regarding copyright ownership.  The ASF licenses this file
    + * to you under the Apache License, Version 2.0 (the
    + * "License"); you may not use this file except in compliance
    + * with the License.  You may obtain a copy of the License at
    + *
    + *   http://www.apache.org/licenses/LICENSE-2.0
    + *
    + * Unless required by applicable law or agreed to in writing,
    + * software distributed under the License is distributed on an
    + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
    + * KIND, either express or implied.  See the License for the
    + * specific language governing permissions and limitations
    + * under the License.
    + *
    + */
    +#include "proton/cpp/ImportExport.h"
    +#include "proton/cpp/ProtonHandle.h"
    +#include "proton/reactor.h"
    +
    +struct pn_connection_t;
    +
    +namespace proton {
    +namespace reactor {
    +
    +class Acceptor : public ProtonHandle<pn_acceptor_t>
    +{
    +  public:
    +    PROTON_CPP_EXTERN Acceptor();
    +    PROTON_CPP_EXTERN Acceptor(pn_acceptor_t *);
    +    PROTON_CPP_EXTERN Acceptor(const Acceptor&);
    --- End diff --
    
    With C++11 onwards need to consider move construction and move assignment too (this is a general point), so if class is movable (and if it is copyable it probably is moveable too) need to add:
    `Class(Class&&)`/`Class& operator=(Class&&)`
    
    Actually though it would be good C++11 style to declare all 5 special functions for every class even if they are default just to show that we really had thought about it:
    ```C++
    class C {
    #ifdef __cplusplus >= 201193L
      C(const C&) = default;               // Copy constructor
      C(C&&) = default;                    // Move constructor
      C& operator=(const C&) & = default;  // Copy assignment operator
      C& operator=(C&&) & = default;       // Move assignment operator
    #endif
      virtual ~C() { }                     // Destructor
    ...
    };
    ```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109372427
  
    There seem to be a couple of extraneous go example files that have snuck into this PR


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109421363
  
    Another general point (I can't exactly see where to attach it in the changes):
    
    As well as using a class hierarchy to get callbacks, we should think about using functions, as C++11 has a nice lambda syntax and std::function too.
    
    Well when I say nice lambda syntax I mean succinct and reasonably readable syntax. `[](){}` anyone?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by astitcher <gi...@git.apache.org>.
Github user astitcher commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31844888
  
    --- Diff: proton-c/bindings/CMakeLists.txt ---
    @@ -109,6 +109,11 @@ if (EMSCRIPTEN_FOUND)
       set (DEFAULT_JAVASCRIPT ON)
     endif (EMSCRIPTEN_FOUND)
     
    +# Prerequisites for C++
    --- End diff --
    
    [Of course I should have known that, as I wrote that foreach logic!]


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on a diff in the pull request:

    https://github.com/apache/qpid-proton/pull/35#discussion_r31840954
  
    --- Diff: examples/cpp/example_test.py ---
    @@ -0,0 +1,105 @@
    +#
    --- End diff --
    
    No, it is a test script that runs the C++ examples and verifies they work.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway commented on the pull request:

    https://github.com/apache/qpid-proton/pull/35#issuecomment-109413137
  
    On Fri, 2015-06-05 at 10:37 -0700, Andrew Stitcher wrote:
    > There seem to be a couple of extraneous go example files that have 
    > snuck into this PR
    > —
    Ooops, removed.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] qpid-proton pull request: C++ binding for proton.

Posted by alanconway <gi...@git.apache.org>.
Github user alanconway closed the pull request at:

    https://github.com/apache/qpid-proton/pull/35


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---