I apologize in advance for not being able to bring a complete review due to my lack of knowledge of the problem domain, but I hope to bring a few elements. I also apologize in advance for the unfair treatment to Luke that I was not able to do the same about Polygon due to my lack of time at the point of the Polygon review. *What is your evaluation of the design? The design looks clear enough so that I was able to understand about what was being done. Adding concept checking is a good idea. It also surely is a modern generic design. I found only 2 points to criticize: - linestring has std::vector as basis class, and therefore it should be clearly documented that this class is not made to be derived from. - most or all of geometries classes have no virtual destructor and thus this should be clearly documented or changed. Better even would be to give the user choice through a policy. *What is your evaluation of the implementation? The code is clean and nice to read and I found it pretty easy to understand (apart from the deep geometry stuff of course). *What is your evaluation of the documentation? The examples are nice to read, the rest of the doc is a bit light and would probably not let me do more complicated stuff. I'd love an example about every feature. *What is your evaluation of the potential usefulness of the library? For me quite big. I'd probably never need the advanced features but the simple ones would solve my biggest problem, which is how to avoid ever having to implement something like this myself, which I would probably never manage anyway. About the completeness of the library, I am not able to judge it and trust the more experienced reviewers. *Did you try to use the library? With what compiler? Did you have any problems? I compiled several examples with VC9 + VC10 beta2. Both compiled without warnings level 3. The VC10 code analyser found a few points (reported to one the library writers), which I suppose are minor. Interestingly, the analyser did not find a bug in the linestring example, which happens only with VC10. This bug needs fixing but must be pretty simple to correct. *How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? I spent a few hours reading and running the examples and checking the analyser and compiler level 4 warnings. I also spent 2 hours looking at the code. *Are you knowledgeable about the problem domain? Not at all. I'm simply looking for a library in case I ever need to do something in this domain. *Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. YES, eventhough I wouldn't count my own opinion as a full vote due to my lack of domain expertise. I like: - some nice features I understood like the linestring and polygon - the fact that the code is clean (meaning lots of care has been spent on it) and that the authors are responsive, which is a sign that the library will be well-maintained. - the examples I like less: - the documentation which gave me the frustrating feeling I missed most of the cool stuff and needs some warning messages for the problems I cited above. Best regards, Christophe Henry
AMDG Christophe Henry wrote:
- most or all of geometries classes have no virtual destructor and thus this should be clearly documented or changed. Better even would be to give the user choice through a policy.
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. In Christ, Steven Watanabe
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. Steven, could you tell me the reason for this? I have always thought
Steven Watanabe wrote: that if you had virtual methods, you should have a virtual destructor since the derived class has something different about it and if being destructed via a reference or pointer to base, the wrong thing could happen without a virtual destructor! Educate me please. I've never heard of any reason to avoid a virtual destructor if you already had virtual methods. Obviously I'm missing something about something I thought I understood. Patrick
AMDG Patrick Horgan wrote:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. Steven, could you tell me the reason for this?
There aren't a lot of virtual function in Boost either. In ggl, the only virtual functions I see are in exception classes.
I have always thought that if you had virtual methods, you should have a virtual destructor since the derived class has something different about it and if being destructed via a reference or pointer to base, the wrong thing could happen without a virtual destructor! Educate me please. I've never heard of any reason to avoid a virtual destructor if you already had virtual methods. Obviously I'm missing something about something I thought I understood.
I don't think so. If you're using virtual functions in the normal OO way, a virtual destructor almost always makes sense. (The use of virtual in Boost.Exception is somewhat different. virtual is being used as an implementation detail to separate a fragment of the interface of a concrete type from the rest of the type in order to minimize dependencies. It isn't being used for polymorphism at all) In Christ, Steven Watanabe
Steven Watanabe wrote:
AMDG
Patrick Horgan wrote:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. Steven, could you tell me the reason for this?
There aren't a lot of virtual function in Boost either. In ggl, the only virtual functions I see are in exception classes.
Another part is where virtual functions are used is extension providing projections, however this part is/was not subject of the review. I can confirm no other types consist of virtual methods, thus no virtual destructors are assumed. Best regards, -- Mateusz Loskot, http://mateusz.loskot.net Charter Member of OSGeo, http://osgeo.org
AMDG
Patrick Horgan wrote:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. Steven, could you tell me the reason for this?
There aren't a lot of virtual function in Boost either. A quick grep in trunk shows about 1200 virtual methods including 136 virtual destructors under boost. In ggl, the only virtual functions I see are in exception classes.
I have always thought that if you had virtual methods, you should have a virtual destructor since the derived class has something different about it and if being destructed via a reference or pointer to base, the wrong thing could happen without a virtual destructor! Educate me please. I've never heard of any reason to avoid a virtual destructor if you already had virtual methods. Obviously I'm missing something about something I thought I understood.
I don't think so. If you're using virtual functions in the normal OO way, a virtual destructor almost always makes sense. (The use of virtual in Boost.Exception is somewhat different. virtual is being used as an implementation detail to separate a fragment of the interface of a concrete type from the rest of the type in order to minimize dependencies. It isn't being used for polymorphism at all) But there are virtual destructors in exception of course (isn't exception pretty, elegant code btw?) I'm still a bit confused. Are you saying that destroying from a base class reference can't be a
Steven Watanabe wrote: problem? Of course that's true if the derived class doesn't ever add any extra dynamic value or system state, but even in that case, surely you'd want to have the virtual destructor anyway if there is a need for virtual functions, so that if someone less thoughtful was extending/maintaining it later, the design would keep those sorts of mistakes from being made. It's defensive programming. But what I was really asking about, was when you said:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. It seems there's lots of virtual destructors in Boost, and you haven't told me why you are against them. In what way are they bad? In this statement you seem to come out against them quite strongly. You seem to be saying that you want it to be a rule that they are not to be used. Am I mis-reading this somehow?
Thanks, Patrick
AMDG Patrick Horgan wrote:
Steven Watanabe wrote:
AMDG
Patrick Horgan wrote:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. Steven, could you tell me the reason for this?
There aren't a lot of virtual function in Boost either. A quick grep in trunk shows about 1200 virtual methods including 136 virtual destructors under boost.
I haven't actually checked, but I my guess is that this is a fairly small fraction of Boost. There are 72 subdirectories in Boost, I think.
In ggl, the only virtual functions I see are in exception classes.
I have always thought that if you had virtual methods, you should have a virtual destructor since the derived class has something different about it and if being destructed via a reference or pointer to base, the wrong thing could happen without a virtual destructor! Educate me please. I've never heard of any reason to avoid a virtual destructor if you already had virtual methods. Obviously I'm missing something about something I thought I understood.
I don't think so. If you're using virtual functions in the normal OO way, a virtual destructor almost always makes sense. (The use of virtual in Boost.Exception is somewhat different. virtual is being used as an implementation detail to separate a fragment of the interface of a concrete type from the rest of the type in order to minimize dependencies. It isn't being used for polymorphism at all) But there are virtual destructors in exception of course (isn't exception pretty, elegant code btw?) I'm still a bit confused. Are you saying that destroying from a base class reference can't be a problem?
In this particular instance destroying through a base class reference is impossible.
Of course that's true if the derived class doesn't ever add any extra dynamic value or system state, but even in that case, surely you'd want to have the virtual destructor anyway if there is a need for virtual functions, so that if someone less thoughtful was extending/maintaining it later, the design would keep those sorts of mistakes from being made. It's defensive programming.
Sorry, I was referring to the specific case of exception_ptr_base where there are virtual functions and a non-virtual destructor.
But what I was really asking about, was when you said:
Steven Watanabe wrote:
FWIW, non-virtual destructors are the norm in Boost. I strongly dislike the idea of having a policy enabling virtual destructors. It seems there's lots of virtual destructors in Boost, and you haven't told me why you are against them.
I'm not. I'm against having a Policy controlling whether a class has a virtual destructor, like this: template<class T, class HasVirtualDestructor = boost::mpl::true_> class optional_virtual_destructor;
In what way are they bad?
They aren't.
In this statement you seem to come out against them quite strongly. You seem to be saying that you want it to be a rule that they are not to be used. Am I mis-reading this somehow?
In Christ, Steven Watanabe
Hi Christoph, Again, thanks for your review and your report.
I like less: - the documentation which gave me the frustrating feeling I missed most of the cool stuff and needs some warning messages for the problems I cited above. We will certainly address the documentation issue.
Regards, Barend P.S. and of course, success with the current review of MSM.
participants (5)
-
Barend Gehrels -
Christophe Henry -
Mateusz Loskot -
Patrick Horgan -
Steven Watanabe