Regarding feedback on OpenWire C++ client

classic Classic list List threaded Threaded
9 messages Options
Reply | Threaded
Open this post in threaded view
|

Regarding feedback on OpenWire C++ client

Mats Forslöf
Hi All,

Thanks everyone for looking at and testing the latest C++ client update. Sorry if we have not been so clear regarding the stability of the code, the code is highly untested and as said in the upload mail we will commence testing/debugging shortly - so expect bugs!!

As we've just finished the port we will shift focus and look into all the issues you have raised, see comments below.

1) Use of smart pointers.

Though the user interface would be cleaner without smart pointers they serves a purpose even when the passed in object is not owned by the client. As soon as an object is shared between two classes you have the problem when the object should be deleted - this problem is eliminated when using SP's, the object is deleted when both object releases its reference to it.

SP's and Strings: Semi-agreed :) The setters should all have "const char*" but we could change the getters to std:string (without SP), then it is up to the user to either use it as a std:string or a "const char*".

2) ITextMessage should extend IMessage

This was a last minute change to make it compile. Nate, your solution seems to be a viable way of handling it - thanks!

3) SP dynamic_cast rather than reinterpret_cast

It is correct that reinterpret_cast<> does not adjust the pointer value as needed when casting between different classes in a class hierarchy. However, the reinterpret_cast<> is used for casting between void* and objects and is needed for the p<> template to work. Where such pointer-value-adjustment is needed (up/down-casting), static_cast<> is used which works the same as dynamic_cast<> except that it does no runtime type check.

4) ExceptionListener

An ExceptionListener is missing. We could either add a method on IMessageListener or add a new interface IExceptionListener, prefer the latter. Ok?

5) .NET namespace

Hiram, what rules does Apache imply on this? Is it ok to use the namespaces your giving examples on? Looking on the Java code all Apache code starts with org.apache.

Regards,
Mats & David
Reply | Threaded
Open this post in threaded view
|

Re: Regarding feedback on OpenWire C++ client

nmittler
Hi Mats,

On 3/27/06, Mats Forslöf <[hidden email]> wrote:
>
> Hi All,
>
> Thanks everyone for looking at and testing the latest C++ client update.
> Sorry if we have not been so clear regarding the stability of the code, the
> code is highly untested and as said in the upload mail we will commence
> testing/debugging shortly - so expect bugs!!


No problem, bugs are expected ... just reporting that I think I found one
:-)

As we've just finished the port we will shift focus and look into all the
> issues you have raised, see comments below.
>
> 1) Use of smart pointers.
>
> Though the user interface would be cleaner without smart pointers they
> serves a purpose even when the passed in object is not owned by the client.
> As soon as an object is shared between two classes you have the problem when
> the object should be deleted - this problem is eliminated when using SP's,
> the object is deleted when both object releases its reference to it.


I'm not sure I agree.  For user-generated objects, it should be up to the
user to add and remove references to them across the openwire library (such
as references to IMessageListeners).  When the user passes in a reference to
one of his objects, he's not expecting to give any amount of lifetime
control to the openwire code.  In fact, the user object may not even have
been created on the heap, it may be a local variable that was added just for
the scope of a function.  In this case, when the openwire library decides to
delete it, the application blows up!  We have to make a separation of what
is and what is not under our library's control.  We can't assume a structure
of the client code, this forces the client into a particular way of
programming and causes our client to become a burden to the developer and
will turn them off to using ActiveMQ.

SP's and Strings: Semi-agreed :) The setters should all have "const char*"
> but we could change the getters to std:string (without SP), then it is up to
> the user to either use it as a std:string or a "const char*".


I think symmetry is a good thing.  We should pick one way or the other ...
either both the getter and setter take const char* or they both take
strings.  Also, when passing strings around, they should be passed as "const
std::string&" ... this way you don't have to copy the entire string, you
just pass a constant reference to it.  This will save a lot of processing
when dealing with large text messages, for example.


2) ITextMessage should extend IMessage
>
> This was a last minute change to make it compile. Nate, your solution
> seems to be a viable way of handling it - thanks!


No problem.

3) SP dynamic_cast rather than reinterpret_cast
>
> It is correct that reinterpret_cast<> does not adjust the pointer value as
> needed when casting between different classes in a class hierarchy. However,
> the reinterpret_cast<> is used for casting between void* and objects and is
> needed for the p<> template to work. Where such pointer-value-adjustment is
> needed (up/down-casting), static_cast<> is used which works the same as
> dynamic_cast<> except that it does no runtime type check.


Hmmm ... so why are we using void* instead of T*?


4) ExceptionListener
>
> An ExceptionListener is missing. We could either add a method on
> IMessageListener or add a new interface IExceptionListener, prefer the
> latter. Ok?


Agreed.

5) .NET namespace
>
> Hiram, what rules does Apache imply on this? Is it ok to use the
> namespaces your giving examples on? Looking on the Java code all Apache code
> starts with org.apache.
>
> Regards,
> Mats & David
>

Regards,
Nate
Reply | Threaded
Open this post in threaded view
|

Re: Regarding feedback on OpenWire C++ client

chirino
In reply to this post by Mats Forslöf
On 3/27/06, Mats Forslöf <[hidden email]> wrote:
>
> 5) .NET namespace
>
> Hiram, what rules does Apache imply on this? Is it ok to use the namespaces your giving examples on? Looking on the Java code all Apache code starts with org.apache.
>

As far as I know the org.apache namespace is a Java thing as it's the
only language the encourages that form.  I've never seen this form use
in other languages yet.

> Regards,
> Mats & David
>


--
Regards,
Hiram
Reply | Threaded
Open this post in threaded view
|

RE: Regarding feedback on OpenWire C++ client

David Fahlander
In reply to this post by Mats Forslöf
Hi Nathan,

> > 1) Use of smart pointers.
> >
> > Though the user interface would be cleaner without smart pointers
they
> > serves a purpose even when the passed in object is not owned by the
client.
> > As soon as an object is shared between two classes you have the
> > problem when the object should be deleted - this problem is
eliminated
> > when using SP's, the object is deleted when both object releases its
reference to it.
>
> I'm not sure I agree.  For user-generated objects, it should be up to
the user to
> add and remove references to them across the openwire library (such as
references
> to IMessageListeners).  When the user passes in a reference to one of
his objects,
> he's not expecting to give any amount of lifetime control to the
openwire code.
> In fact, the user object may not even have been created on the heap,
it may be a
> local variable that was added just for the scope of a function.  In
this case,
> when the openwire library decides to delete it, the application blows
up!  We
> have to make a separation of what is and what is not under our
library's control.
> We can't assume a structure of the client code, this forces the client
into a
> particular way of programming and causes our client to become a burden
to the
> developer and will turn them off to using ActiveMQ.

Regarding objects that the user provides, we still get benefits by using
smart pointers for them. It is not always the creator that is
responsible of deletion. Especially in a threaded application it can be
important to not end-up with ghost pointers. A thread in our client
library that holds a pointer to a listener, must be able to rely on that
the instance is still alive as long as it holds the SP to it. If the
user would provide an ordinary pointer, the user must never delete it as
long as there might be threads in our library using it. What about
"static" objects that should last forever in the client's lifetime then?
Still, we want to be able to take down the client in a controlled way.
Doing that without smart pointers, may create a lot of problems in our
experience.

The "burden" for the user would only lie in the classes that implement
our interfaces. If our API requires a p<IMessageListener>, the user must
hold his message listener using a p<MyMessageListener> allocated with
new MyMessageLister (...);. The user does not have to let this affect
his design. If the user wants, this instance can lie as a member within
another class that is neither derived from our interface nor maintained
as a smart pointer.

> > SP's and Strings: Semi-agreed :) The setters should all have "const
char*"
> > but we could change the getters to std:string (without SP), then it
is
> > up to the user to either use it as a std:string or a "const char*".
>
> I think symmetry is a good thing.  We should pick one way or the other
...
> either both the getter and setter take const char* or they both take
strings.
> Also, when passing strings around, they should be passed as "const
std::string&" ...
> this way you don't have to copy the entire string, you just pass a
constant reference
> to it.  This will save a lot of processing when dealing with large
text messages, for
> example.

The options we have regarding passing and holding strings are the
following:

Class Xxx {
  p<string> name;
public:
  void setName (p<string> name) {
    this->name = name;
  }
  p<string> getName () {
    return this->name;
  }
  p<string> createName() {
    return new string ("name: " + *name);
  }
};

In the above example, strings are never copied, just the ptr value. The
downside is that the caller needs to pass their strings as p<string>.
This version adds the least overhead.

---

Class Xxx {
  string name;
  void setName (const string& name) {
    this->name = name;
  }
  const string& getName () {
    return this->name;
  }
  string createName() {
    return string ("name: " + name);
  }
};

In this example the string is always copied in the setters but not in
getters.

---

Class Xxx {
  const char* name;
  void setName (const char* name) {
    this->name = name;
  }
  const char* getName () {
    return this->name;
  }
  const char* () {
    const char* val = new char[256];
    strcpy (val, "name: ");
    strcat (val, name);
    return val;
  }
};

This example is unacceptable since ownership is undefined.

We are open to use any of the first two examples.

Regards,
David & Mats
Reply | Threaded
Open this post in threaded view
|

RE: Regarding feedback on OpenWire C++ client

Mats Forslöf
In reply to this post by Mats Forslöf
Hi Hiram,

>> 5) .NET namespace
>>
>> Hiram, what rules does Apache imply on this? Is it ok to use the namespaces your giving examples on? Looking on the Java code all Apache code starts with org.apache.
>>
>
>As far as I know the org.apache namespace is a Java thing as it's the only language the encourages that form.  I've never seen this form use in other languages yet.

Even so it might be better to be a bit overzealous to prevent namespace clashes :)

If ASF does not have any rules about this we'll change it - no problem. However, if we're to sync the names between C# and C++ the C# name spaces should be altered also!?

C#
namespace cms
namespace activemq.command
namespace activemq.core
namespace activemq.io

C++
namespace cms
namespace activemq::command
namespace activemq::io
namespace activemq::marshal
namespace activemq::protocol
namespace activemq::transport
namespace activemq::util

Regards,
Mats
Reply | Threaded
Open this post in threaded view
|

Re: Regarding feedback on OpenWire C++ client

nmittler
In reply to this post by David Fahlander
On 3/28/06, David Fahlander <[hidden email]> wrote:

>
> Hi Nathan,
>
> > > 1) Use of smart pointers.
> > >
> > > Though the user interface would be cleaner without smart pointers
> they
> > > serves a purpose even when the passed in object is not owned by the
> client.
> > > As soon as an object is shared between two classes you have the
> > > problem when the object should be deleted - this problem is
> eliminated
> > > when using SP's, the object is deleted when both object releases its
> reference to it.
> >
> > I'm not sure I agree.  For user-generated objects, it should be up to
> the user to
> > add and remove references to them across the openwire library (such as
> references
> > to IMessageListeners).  When the user passes in a reference to one of
> his objects,
> > he's not expecting to give any amount of lifetime control to the
> openwire code.
> > In fact, the user object may not even have been created on the heap,
> it may be a
> > local variable that was added just for the scope of a function.  In
> this case,
> > when the openwire library decides to delete it, the application blows
> up!  We
> > have to make a separation of what is and what is not under our
> library's control.
> > We can't assume a structure of the client code, this forces the client
> into a
> > particular way of programming and causes our client to become a burden
> to the
> > developer and will turn them off to using ActiveMQ.
>
> Regarding objects that the user provides, we still get benefits by using
> smart pointers for them. It is not always the creator that is
> responsible of deletion. Especially in a threaded application it can be
> important to not end-up with ghost pointers. A thread in our client
> library that holds a pointer to a listener, must be able to rely on that
> the instance is still alive as long as it holds the SP to it. If the
> user would provide an ordinary pointer, the user must never delete it as
> long as there might be threads in our library using it. What about
> "static" objects that should last forever in the client's lifetime then?
> Still, we want to be able to take down the client in a controlled way.
> Doing that without smart pointers, may create a lot of problems in our
> experience.
>
> The "burden" for the user would only lie in the classes that implement
> our interfaces. If our API requires a p<IMessageListener>, the user must
> hold his message listener using a p<MyMessageListener> allocated with
> new MyMessageLister (...);. The user does not have to let this affect
> his design. If the user wants, this instance can lie as a member within
> another class that is neither derived from our interface nor maintained
> as a smart pointer.


You get no benefits at all by adding smart pointers to the interface.  The
user will delete their objects whenever they wish, regardless of what the
openwire library does ... and if it does so, the openwire library will
explode as soon as it tries to reference it.  That's the thing - this is C++
and there is no mechanism to prevent someone from coding badly, and adding
smart pointers to the API isn't going to change that.  The user is
responsible for both adding and removing referenes to its objects.  And it's
not the job of the openwire lib to worry about memory management in the
user-domain - it should only be concerned with its own memory management.

In addition, smart pointers are assuming that the object is allocated on the
heap, as it will do a "delete" when the last reference is removed.  This is
wrong, because the user should be able allocate its objects in whatever way
makes sense for the application.

Also, adding smart pointer arguments to all the methods on the api makes it
complicated and ugly.  If the openwire library wants to use smart pointers
internally, that's fine, but it shouldn't impose the use of smart pointers
on the user.  It's our job to make the user's experience a good one so they
continue to use ActiveMQ in their applications.


> > SP's and Strings: Semi-agreed :) The setters should all have "const
> char*"
> > > but we could change the getters to std:string (without SP), then it
> is
> > > up to the user to either use it as a std:string or a "const char*".
> >
> > I think symmetry is a good thing.  We should pick one way or the other
> ...
> > either both the getter and setter take const char* or they both take
> strings.
> > Also, when passing strings around, they should be passed as "const
> std::string&" ...
> > this way you don't have to copy the entire string, you just pass a
> constant reference
> > to it.  This will save a lot of processing when dealing with large
> text messages, for
> > example.
>
> The options we have regarding passing and holding strings are the
> following:
>
> Class Xxx {
>   p<string> name;
> public:
>   void setName (p<string> name) {
>     this->name = name;
>   }
>   p<string> getName () {
>     return this->name;
>   }
>   p<string> createName() {
>     return new string ("name: " + *name);
>   }
> };
>
> In the above example, strings are never copied, just the ptr value. The
> downside is that the caller needs to pass their strings as p<string>.
> This version adds the least overhead.


It's all about the user and helping them come up to speed and use the api as
quickly and painlessly as possible.  I understand that in these cases you're
passing a pointer and not copying, but it complicates the user interface
when the person just wants to pass in a string.  They shouldn't have to
create a string on the heap an then wrap it in a smart pointer to just call
a function.

---

>
> Class Xxx {
>   string name;
>   void setName (const string& name) {
>     this->name = name;
>   }
>   const string& getName () {
>     return this->name;
>   }
>   string createName() {
>     return string ("name: " + name);
>   }
> };
>
> In this example the string is always copied in the setters but not in
> getters.
>
> ---
>
> Class Xxx {
>   const char* name;
>   void setName (const char* name) {
>     this->name = name;
>   }
>   const char* getName () {
>     return this->name;
>   }
>   const char* () {
>     const char* val = new char[256];
>     strcpy (val, "name: ");
>     strcat (val, name);
>     return val;
>   }
> };
>
> This example is unacceptable since ownership is undefined.


I think you were missing my point.   I would change the example to this:

 Class Xxx {
  std::string name;
  void setName (const char* name) {
    this->name = name;
  }
  const char* getName () const{
    return this->name.c_str();
  }
};

OR ...

Class Xxx {
  std::string name;
  void setName (const std::string& name) {
    this->name = name;
  }
  const std::string& getName () const {
    return this->name;
  }
};

This way there is one copy done in the setter, but this bridges the gap
between the user domain and our lib.  Once that's been done, the user can
access the variable via the getter all day without copying.  This is a
standard way of handling strings in an application.

We are open to use any of the first two examples.
>
> Regards,
> David & Mats
>

Regards,
Nate
Reply | Threaded
Open this post in threaded view
|

RE: Regarding feedback on OpenWire C++ client

Mats Forslöf
In reply to this post by Mats Forslöf
Nate,

>You get no benefits at all by adding smart pointers to the interface.
>The user will delete their objects whenever they wish, regardless of
>what the openwire library does ... and if it does so, the openwire
>library will explode as soon as it tries to reference it.  That's the
>thing - this is C++ and there is no mechanism to prevent someone from
>coding badly, and adding smart pointers to the API isn't going to change
>that. The user is responsible for both adding and removing referenes to
>its objects.  And it's not the job of the openwire lib to worry about
>memory management in the user-domain - it should only be concerned with
>its own memory management.

>In addition, smart pointers are assuming that the object is allocated on
>the heap, as it will do a "delete" when the last reference is removed.
>This is wrong, because the user should be able allocate its objects in
>whatever way makes sense for the application.

>Also, adding smart pointer arguments to all the methods on the api makes
>it complicated and ugly.  If the openwire library wants to use smart pointers
>internally, that's fine, but it shouldn't impose the use of smart pointers
>on the user.  It's our job to make the user's experience a good one so they
>continue to use ActiveMQ in their applications.

Please define the interfaces that you want to be SP-free!

>It's all about the user and helping them come up to speed and use the api
>as quickly and painlessly as possible.  I understand that in these cases
>you're passing a pointer and not copying, but it complicates the user
>interface when the person just wants to pass in a string.  They shouldn't
>have to create a string on the heap an then wrap it in a smart pointer to
>just call a function.

Just a quick note, this is not necessary with the current design of accepting "const char*" and returning p<string>. The user can easily extract the const char from the SP string if wanted.

>Class Xxx {
>  std::string name;
>  void setName (const std::string& name) {
>    this->name = name;
>  }
>  const std::string& getName () const {
>    return this->name;
>  }
>};

Shall we agree on the one above?

Regards,
Mats & David

Reply | Threaded
Open this post in threaded view
|

RE: Regarding feedback on OpenWire C++ client

Mittler, Nathan
In reply to this post by Mats Forslöf

>Please define the interfaces that you want to be SP-free!

I'm with Hiram ... I think the entire API should be SP-free.  The less
the users have to see SPs, the better.


>>It's all about the user and helping them come up to speed and use the
api
>>as quickly and painlessly as possible.  I understand that in these
cases
>>you're passing a pointer and not copying, but it complicates the user
>>interface when the person just wants to pass in a string.  They
shouldn't
>>have to create a string on the heap an then wrap it in a smart pointer
to
>>just call a function.

>Just a quick note, this is not necessary with the current design of
>accepting "const char*" and returning p<string>. The user can easily
>extract the const char from the SP string if wanted.
>

But then you lose the symmetry of the getters and setters ... the setter
should take the same type that the getter returns.  IMHO, it seems
strange to have the setter take a char* and then the getter returns a
smart pointer.

>>Class Xxx {
>>  std::string name;
>>  void setName (const std::string& name) {
>>    this->name = name;
>>  }
>>  const std::string& getName () const {
>>    return this->name;
>>  }
>>};
>
>Shall we agree on the one above?
>
>Regards,
>Mats & David

Regards,
Nate
Reply | Threaded
Open this post in threaded view
|

Re: Regarding feedback on OpenWire C++ client

chirino
In reply to this post by Mats Forslöf
On 3/28/06, Mats Forslöf <[hidden email]> wrote:
> Hi Hiram,
>
>
> Even so it might be better to be a bit overzealous to prevent namespace clashes :)
>

I would like to make this c++ lib, feel very c++ like, I don't want it
to smell like it came from a Java shop.  So I think it's better to
have short names like most of the other libs out there.

> If ASF does not have any rules about this we'll change it - no problem. However, if we're to sync the names between C# and C++ the C# name spaces should be altered also!?
>
> C#
> namespace cms
> namespace activemq.command
> namespace activemq.core
> namespace activemq.io
>


No.. becaue the C# one alos works for VB.NET and J# etc. etc. that's
why the interface are in the .NET Message Service (NMS) namespace.
Also .Net uses Camel case convention in it's namespaces.  So we will
do it like that.  Once again the aim is to have a .NET library the
feels very native.


> C++
> namespace cms
> namespace activemq::command
> namespace activemq::io
> namespace activemq::marshal
> namespace activemq::protocol
> namespace activemq::transport
> namespace activemq::util
>

This looks good to me!

> Regards,
> Mats
>


--
Regards,
Hiram