Re: simplify mod_perl.c a bit

[Philippe M. Chiasson]

On 10-03-31 11:07 , Torsten Förtsch wrote:
> Hi,
> 
> the patch below simplifies mod_perl.c a bit.
> 
> Instead of
> 
> modperl_response_handler_run(r, finish) {
>   do something
>   if( finish ) {
>     modperl_response_finish()
>   }
> }
> 
> and calling that function in one place as
> 
> modperl_response_handler_run(r, TRUE)
> 
> and in the second place as
> 
> modperl_response_handler_run(r, TRUE)
> do something
> modperl_response_finish()
> 
> it now looks like
> 
> modperl_response_handler_run(r) {
>   do something
> }
> 
> 1st usage:
> modperl_response_handler_run(r)
> modperl_response_finish()
> 
> 2nd usage:
> modperl_response_handler_run(r)
> do something
> modperl_response_finish()

Nice, thanks, and a good idea. Functions that take some flag only to
trigger different behaviour are usually bad form. Below is a slightly
different patch do address a simple nit.

In ANSI C you can't define variables in blocks, like

int foo(void) {
  int a = 2;
  some code
  {
    int b = 3;
    more code
  }

So the below patch fixes that by making the rc status variable global
to the function. I compile with -Wall -Werror and it tends to pick these
up.

Nice side-effect is that it ends up simplifying a little further the
case where calling modperl_response_finish() is necessary.

Otherwise, +1

Index: src/modules/perl/mod_perl.c
===================================================================
--- src/modules/perl/mod_perl.c (revision 930351)
+++ src/modules/perl/mod_perl.c (working copy)
@@ -991,7 +991,7 @@
     return modperl_wbucket_flush(rcfg->wbucket, FALSE);
 }

-static int modperl_response_handler_run(request_rec *r, int finish)
+static int modperl_response_handler_run(request_rec *r)
 {
     int retval;

@@ -1003,13 +1003,6 @@
         r->handler = r->content_type; /* let http_core or whatever try */
     }

-    if (finish) {
-        apr_status_t rc = modperl_response_finish(r);
-        if (rc != APR_SUCCESS) {
-            retval = rc;
-        }
-    }
-
     return retval;
 }

@@ -1019,7 +1012,7 @@
 #ifdef USE_ITHREADS
     MP_dRCFG;
 #endif
-    apr_status_t retval;
+    apr_status_t retval, rc;

 #ifdef USE_ITHREADS
     pTHX;
@@ -1043,7 +1036,11 @@
         modperl_env_request_populate(aTHX_ r);
     }

-    retval = modperl_response_handler_run(r, TRUE);
+    retval = modperl_response_handler_run(r);
+    rc = modperl_response_finish(r);
+    if (rc != APR_SUCCESS) {
+        retval = rc;
+    }

 #ifdef USE_ITHREADS
     if (MpInterpPUTBACK(interp)) {
@@ -1099,7 +1096,7 @@

     modperl_env_request_tie(aTHX_ r);

-    retval = modperl_response_handler_run(r, FALSE);
+    retval = modperl_response_handler_run(r);

     modperl_env_request_untie(aTHX_ r);

-- 
Philippe M. Chiasson     GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/       m/gozer\@(apache|cpan|ectoplasm)\.org/

signature.asc
Description: OpenPGP digital signature