On Friday 20 July 2001 10:30, Aaron Bannert wrote:
> I submitted most of this patch last weekend. Since then I've fixed
> a small bug in the apr_thread_exit() patch I made (I could really
> use APR commit access for the little things like this :). I haven't
> heard any objections (Ryan ;), so I assume these changes were
> acceptable.
Grrrrrr...... This patch does all sorts of stuff that it doesn't need to do. :-(
The BeOS code was changed drastically, although most of the changes are more cosmetic
than anything else, some are more serious. I have decided not to apply this patch
because some of the changes are just not necessary, and others are just wrong given
APR's general coding style. I have more comments in-line.
> Index: srclib/apr/threadproc/beos/thread.c
> ===================================================================
> RCS file: /home/cvspublic/apr/threadproc/beos/thread.c,v
> retrieving revision 1.22
> diff -u -r1.22 thread.c
> --- srclib/apr/threadproc/beos/thread.c 2001/06/14 18:52:05 1.22
> +++ srclib/apr/threadproc/beos/thread.c 2001/07/20 17:11:33
> @@ -94,13 +94,17 @@
> {
> int32 temp;
> apr_status_t stat;
> + struct thread_info_t *thr_info;
What is this variable for? It's never used anywhere.
> + apr_thread_t *thread;
> + apr_thread_param_t *thrparm;
>
> - (*new) = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> - if ((*new) == NULL) {
> + thread = (apr_thread_t *)apr_palloc(cont, sizeof(apr_thread_t));
> + (*new) = thread;
Why did we add a new variable here? A BIG portion of this patch could
have been avoided, making it easier to review, if variable name changes
like this hadn't been included.
> apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> *thd) {
> - if (wait_for_thread(thd->td,(void *)&retval) == B_NO_ERROR) {
> + apr_status_t *thrstat = NULL;
> +
> + if (wait_for_thread(thd->td,(void *)&thrstat) == B_NO_ERROR) {
> + if (retval != NULL && thrstat != NULL)
> + *retval = *thrstat;
What is this change about? If the user passed in NULL for retval, then we
should just seg fault here. Why was this all added?
> - *retval = (apr_status_t)thd->rv;
> + if (retval != NULL)
> + *retval = (apr_status_t)thd->rv;
Again, we don't need this change.
> Index: srclib/apr/threadproc/unix/thread.c
> ===================================================================
> RCS file: /home/cvspublic/apr/threadproc/unix/thread.c,v
> retrieving revision 1.39
> diff -u -r1.39 thread.c
> --- srclib/apr/threadproc/unix/thread.c 2001/06/14 18:52:09 1.39
> +++ srclib/apr/threadproc/unix/thread.c 2001/07/20 17:11:33
> @@ -122,32 +122,43 @@
> {
> apr_status_t stat;
> pthread_attr_t *temp;
> + apr_thread_t *thread;
> + apr_thread_param_t *thrparm;
>
> - (*new) = (apr_thread_t *)apr_pcalloc(cont,
> sizeof(apr_thread_t)); + thread = (apr_thread_t
> *)apr_pcalloc(cont, sizeof(apr_thread_t)); + (*new) = thread;
>
> - if ((*new) == NULL) {
> + if (thread == NULL) {
Same as above. Changing variable names is not a good thing to
do in the middle of a patch this size.
> @@ -178,8 +189,11 @@
> apr_status_t apr_thread_join(apr_status_t *retval, apr_thread_t
> *thd) {
> apr_status_t stat;
> + apr_status_t *thrstat = NULL;
>
> - if ((stat = pthread_join(*thd->td,(void *)&retval)) == 0) {
> + if ((stat = pthread_join(*thd->td,(void **)&thrstat)) == 0) {
> + if (retval != NULL && thrstat != NULL)
> + *retval = *thrstat;
And again, this could have been fixed by just adding a * to the (void *), couldn't it?
I have skipped the Windows section of the patch, but it is more of the same.
Fix those comments, and I will review the patch again. MNSO is that the goal of
patches should be to change as little code as possible. The more changes there are,
the harder it is to review the patch. If a patch is generated, and there isn't a need
for
a line of code to have changed, the person who created the patch should go through
and ask why that line was changed.
Ryan
_____________________________________________________________________________
Ryan Bloom [EMAIL PROTECTED]
Covalent Technologies [EMAIL PROTECTED]
-----------------------------------------------------------------------------
