You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Joe Orton <jo...@redhat.com> on 2010/12/02 10:39:34 UTC
Re: svn commit: r1040177 -
/httpd/httpd/trunk/modules/http/http_protocol.c
On Mon, Nov 29, 2010 at 04:37:49PM -0000, fuankg@apache.org wrote:
> URL: http://svn.apache.org/viewvc?rev=1040177&view=rev
> Log:
> Supress compiler warning.
...
> ==============================================================================
> --- httpd/httpd/trunk/modules/http/http_protocol.c (original)
> +++ httpd/httpd/trunk/modules/http/http_protocol.c Mon Nov 29 16:37:49 2010
> @@ -204,7 +204,12 @@ AP_DECLARE(int) ap_set_keepalive(request
> * THEN we can be persistent, which requires more headers be output.
> *
> * Note that the condition evaluation order is extremely important.
> + *
> + * Silent compiler warnings for (r->chunked = 1) with #pragma
> */
> +#ifdef __WATCOMC__
> +#pragma disable_message(105)
> +#endif
Eww. Do you really need to litter the source code with this stuff? Can
you not flip compiler switches in the Makefiles? Regards, Joe
Re: svn commit: r1040177 - /httpd/httpd/trunk/modules/http/http_protocol.c
Posted by "Malte S. Stretz" <ms...@apache.org>.
On Friday 03 December 2010 09:52:06 Guenter Knauf wrote:
> Am 02.12.2010 10:39, schrieb Joe Orton:
> > On Mon, Nov 29, 2010 at 04:37:49PM -0000, fuankg@apache.org wrote:
> >> +#ifdef __WATCOMC__
> >> +#pragma disable_message(105)
> >> +#endif
> >
> > Eww. Do you really need to litter the source code with this stuff?
> > Can you not flip compiler switches in the Makefiles? Regards, Joe
>
> no. If I use a compiler switch it aplies for all source files which is
> certainly not what I want - instead I only want to suppress this one
> warning about assignment inside condition where the comment states that
> its ugly but wanted, while at all other places the very same warning is
> important and useful since you can just acciedently miss a 2nd '='
> where you dont want to assign but only compare.
> I even expect to add more pragmas for other compilers, f.e. gcc, at
> this place if we would compile with all possible warnings (maybe -Wall
> is already enough).
I recently stumbled upon that big condition and ran away screaming.
Wouldn't it be better just to refactor it so it is better to read and
debug? Some thing like the following (untested, just hacked down)
shouldn't be more inefficient and even shows that some cases like the
check for HTTP/1.1 is done twice and could be factored out. It also shows
when the chunked flag is actually set (because the last check is never
evaluated if all the other or'ed stuff is already true, if I got this
right).
----8<----
int ka = 1;
ka &&= r->connection->keepalive != AP_CONN_CLOSE;
ka &&= !r->expecting_100;
if (!((r->status == HTTP_NOT_MODIFIED)
|| (r->status == HTTP_NO_CONTENT)
|| !r->expecting_100
|| r->header_only
|| apr_table_get(r->headers_out, "Content-Length")
|| ap_find_last_token(r->pool,
apr_table_get(r->headers_out, "Transfer-Encoding"),
"chunked"))) {
if (r->proto_num >= HTTP_VERSION(1,1)) {
r->chunked = 1;
}
else {
ka = 0;
}
}
ka &&= r->server->keep_alive;
ka &&= r->server->keep_alive_timeout > 0;
ka &&= (r->server->keep_alive_max == 0) || (left > 0);
ka &&= !ap_status_drops_connection(r->status);
ka &&= !wimpy;
ka &&= !ap_find_token(r->pool, conn, "close");
ka &&= (!apr_table_get(r->subprocess_env, "nokeepalive")
|| apr_table_get(r->headers_in, "Via"));
ka && ((ka_sent = ap_find_token(r->pool, conn, "keep-alive"))
|| (r->proto_num >= HTTP_VERSION(1,1)));
ka &&= is_mpm_running());
if (ka) { //...
----8<----
Cheers,
Malte
Re: svn commit: r1040177 - /httpd/httpd/trunk/modules/http/http_protocol.c
Posted by Guenter Knauf <fu...@apache.org>.
Am 03.12.2010 15:26, schrieb Joe Orton:
> This is pretty ugly, please don't. If you think this warning from this
> compiler matters so much, set up some kind of CI system which alerts you
> when they are introduced. Littering platform-independent parts of the
> source with ugly #pragmas is not improving the quality of the source
> code in any way.
agreed --> r1041905.
Gün.
Re: svn commit: r1040177 -
/httpd/httpd/trunk/modules/http/http_protocol.c
Posted by Joe Orton <jo...@redhat.com>.
On Fri, Dec 03, 2010 at 09:52:06AM +0100, Guenter Knauf wrote:
> Am 02.12.2010 10:39, schrieb Joe Orton:
> >On Mon, Nov 29, 2010 at 04:37:49PM -0000, fuankg@apache.org wrote:
> >>URL: http://svn.apache.org/viewvc?rev=1040177&view=rev
> >>Log:
> >>Supress compiler warning.
> >...
> >>==============================================================================
> >>--- httpd/httpd/trunk/modules/http/http_protocol.c (original)
> >>+++ httpd/httpd/trunk/modules/http/http_protocol.c Mon Nov 29 16:37:49 2010
> >>@@ -204,7 +204,12 @@ AP_DECLARE(int) ap_set_keepalive(request
> >> * THEN we can be persistent, which requires more headers be output.
> >> *
> >> * Note that the condition evaluation order is extremely important.
> >>+ *
> >>+ * Silent compiler warnings for (r->chunked = 1) with #pragma
> >> */
> >>+#ifdef __WATCOMC__
> >>+#pragma disable_message(105)
> >>+#endif
> >
> >Eww. Do you really need to litter the source code with this stuff? Can
> >you not flip compiler switches in the Makefiles? Regards, Joe
> no. If I use a compiler switch it aplies for all source files which
> is certainly not what I want - instead I only want to suppress this
> one warning about assignment inside condition where the comment
> states that its ugly but wanted, while at all other places the very
> same warning is important and useful since you can just acciedently
> miss a 2nd '=' where you dont want to assign but only compare.
> I even expect to add more pragmas for other compilers, f.e. gcc, at
> this place if we would compile with all possible warnings (maybe
> -Wall is already enough).
This is pretty ugly, please don't. If you think this warning from this
compiler matters so much, set up some kind of CI system which alerts you
when they are introduced. Littering platform-independent parts of the
source with ugly #pragmas is not improving the quality of the source
code in any way.
Regards, Joe
Re: svn commit: r1040177 - /httpd/httpd/trunk/modules/http/http_protocol.c
Posted by Guenter Knauf <fu...@apache.org>.
Am 02.12.2010 10:39, schrieb Joe Orton:
> On Mon, Nov 29, 2010 at 04:37:49PM -0000, fuankg@apache.org wrote:
>> URL: http://svn.apache.org/viewvc?rev=1040177&view=rev
>> Log:
>> Supress compiler warning.
> ...
>> ==============================================================================
>> --- httpd/httpd/trunk/modules/http/http_protocol.c (original)
>> +++ httpd/httpd/trunk/modules/http/http_protocol.c Mon Nov 29 16:37:49 2010
>> @@ -204,7 +204,12 @@ AP_DECLARE(int) ap_set_keepalive(request
>> * THEN we can be persistent, which requires more headers be output.
>> *
>> * Note that the condition evaluation order is extremely important.
>> + *
>> + * Silent compiler warnings for (r->chunked = 1) with #pragma
>> */
>> +#ifdef __WATCOMC__
>> +#pragma disable_message(105)
>> +#endif
>
> Eww. Do you really need to litter the source code with this stuff? Can
> you not flip compiler switches in the Makefiles? Regards, Joe
no. If I use a compiler switch it aplies for all source files which is
certainly not what I want - instead I only want to suppress this one
warning about assignment inside condition where the comment states that
its ugly but wanted, while at all other places the very same warning is
important and useful since you can just acciedently miss a 2nd '=' where
you dont want to assign but only compare.
I even expect to add more pragmas for other compilers, f.e. gcc, at this
place if we would compile with all possible warnings (maybe -Wall is
already enough).
Gün.