On 25/02/12 12:41 +0100, Vicente J. Botet Escriba wrote:
Le 24/02/12 15:57, Brett Lentz a écrit :
Hi folks!
It's been a few months since I last mailed the list. I've been working on bringing some forked changes from the Passenger project into Boost due to their more general applicability.
With some help, I've got a working patch against SVN trunk that enables one of Passenger's more useful features: obtaining exception stacktraces all the way through the stack.
However, due to the size of the patch and some namespacing choices made by the Phusion people, I'd like to get some feedback on this patch to ensure it's suitable for being brought into Boost.
Hi,
I wanted just to note that this patch concerns the libraries Boost.Exception, Boost.System and Boost.Thread and that it is a library without a name. Could it be possible to define these functionalities in independent libraries?
First, there's some helper classes that Phusion has put in an "oxt" namespace that I'm not really sure where they should be put or if suitable analogues already exist within Boost. For the current version of the patch, these files are simply put into 'boost/oxt' until I have a better location for them. Which libraries are using it in your patch. Maybe you can put it on boost::detail::oxt?
I can do that or, as you mention later in your reply, I can use the spin lock implementation that already exists in boost.
Second, the current patch uses boost::tracable_exception. I'd like to confirm whether that is acceptable or whether I should move this to a different location in the boost namespace.
Finally, any other comments about this patch are welcome. I have attached both the patch, and some example code that will demonstrate the functionality.
I don't know what is your intention proposing this patch, but I don't think the patch in its actual form could be applied to Boost. Its is a big patch changing a lot of code on existing libraries without any test, neither documentation.
My personal goal is to get Passenger to the point where it can compile against the vanilla Boost libraries with minimal or no loss in functionality. This is a prerequisite for its inclusion into Fedora and is very useful for a few different reasons. Passenger is something a few different people have been trying to get into Fedora for quite a while now. I'm just trying to help the process along. (For the full history, see: https://bugzilla.redhat.com/show_bug.cgi?id=470696)
I believe that the changes that Passenger currently ships in their bundled version of Boost are useful and generally applicable. Other users of Boost can benefit from the inclusion of features like fully traceable exceptions. I have no doubt of the utility of a backtrace library. The usual way is to propose a library for review.
There is a little bit of documentation in boost/exception/backtrace.hpp, but I'll see about getting some test cases created. I read them. More like that will help to understand user interface and
Le 27/02/12 16:41, Brett Lentz a écrit : the design.
* I don't see any changes on windows files. Have you an implementation for windows?
I do not have a Windows implementation. That platform isn't the Passenger folks' focus, nor is it mine. If this is an issue that will prevent this feature from being included, I can see about adding Windows support to the patch.
I don't know the effort to get the code portable on windows platform, but don't providing it is a show-stopper to his adoption.
Note that the patch contains the files
Index: boost/exception/tracable_exception.hpp
Index: libs/thread/src/pthread/tracable_thread.cpp
That is, the declaration is in Boost.Exception, while the implementation is in Boost.Thread :(
I'm open to suggestions on how you think it should be implemented. Passenger is highly multi-threaded server software, so that's the focus. If we need to make a change to make this feature more generally useful, it's something we can consider.
What I meant, is that will expect that the implementation of the file boost/exception/tracable_exception.hpp to be in the exception library. Anyway, it will be better if you can put all in a separated library, e.g? BackTrace
* Is it necessary that tracable_thread inherits from thread? What about adding a tracker that can be used on any thread?
Here again, it would help if you could define a tracable_thread or a tracker object, that doesn't need the modification of boost::thread. You could make traceable_thread a wrapper of boost::thread or use a thread specific Tracker object.
* Why thread_exception don't inherits now from boost::system_error? This breaks the current exception interface. Is it absolutely needed that thread_exception inherits from tracable_exception to provide tracable exceptions?
The point of doing it this was was to have Boost.Thread throw exceptions that are derived from thread_exception. Doing it another way doesn't provide the same ability to trace the full length of the stack. I guess that any library will be free make its exceptions to inherit from traceable_exception, and this should not imply to lost the its current interface. All the thread_exception, must inherit from boost::system_error.
* There is already a spin lock implementation in Boost detail. Have you see if it responds to your needs?
This is something I will investigate. The current page is lifted from the Passenger code as closely to how it exists there. Our first goal was just to get a working patch that can be applied to demonstrate the functionality. Now that we've got that, I want to start figuring out what issues will prevent inclusion into Boost.
* I propose you to make a concrete proposal for the macros LIKELY and UNLIKELY to Boost.Config.
I'll do that. Being that this gets into compiler-specific and/or platform-specific territory, can you or anyone else comment on what minimum platforms need to be supported for inclusion? As this is an optimization feature, I think it will be enough to provide it for the compilers supporting it. The questions is if the proposed macros works for all the compiler proposing a similar feature.
* I don't know if the license is compatible with the Boost license. Could you use the Boost license?
Passenger is using the stock MIT License which appears to be what the Boost License is an extension of. I can ping the Passenger guys to see if the additional text in the second paragraph of the Boost License is acceptable to them. However, in my non-legal opinion, these licenses should be compatible as they're substantially the same text. AFAIK, every Boost file must contain the Boost license.
Thanks so much for the comments. I'll be working on making the changes you suggest. Hopefully I'll have a new patch set to post to the list soon.
Please, try to convert the patch in one or several separated libraries or specific patches for each library. It will much easier to get at least some of them accepted. If you don't reach to get them included into Boost, you could always add them as part of Passenger. Best, Vicente