Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
MIME-Version: 1.0
In-Reply-To: <3eb43047-379e-781d-f51f-3f84c10e5681@gmail.com>
References: <20170620230820.92CF93A25B1@svn01-us-west.apache.org>
 <CACsi252iUnj4K_CBzDcLhTP2tWO4T1-9LxjesNMKFrqk6_=TEw@mail.gmail.com> <3eb43047-379e-781d-f51f-3f84c10e5681@gmail.com>
From: William A Rowe Jr <wrowe@rowe-clan.net>
Date: Mon, 26 Jun 2017 16:05:55 -0500
Message-ID: <CACsi253d+AZuWK5f4TU-7ATMQCS=QuH6Cs9oBmMEzALFkMykPA@mail.gmail.com>
Subject: Re: svn commit: r1799375 - /httpd/httpd/trunk/server/util.c
To: httpd <dev@httpd.apache.org>
Content-Type: text/plain; charset="UTF-8"
archived-at: Mon, 26 Jun 2017 21:06:15 -0000

On Mon, Jun 26, 2017 at 3:14 PM, Jacob Champion <champion.p@gmail.com> wrote:
> On 06/20/2017 11:08 PM, William A Rowe Jr wrote:
>>
>> Sorry but I reraise my objection and veto worthless cpu cycles.
>
> For posterity, can I get a succinct description of your technical
> justification for this veto?

I have several.

 * Wasting CPU cycles for a no-op *which the compiler cannot
   recover from is unacceptable. httpd is coded in C for a reason,
   with all the associated side effects. Any deoptimization as some
   matter of style or as a matter of security (e.g. stack -> alloc) is
   discussed on list and the costs weighed by the group.

 * The test_char.h table is a private-use entity with 256 well-defined
   octets (not utf-8, not char-wise, these are raw bytes). Sometimes,
   and often for some platform, one is wrong, it is simply fixed, and
   the sole consumer, util.c, is corrected.

 * The test_char.h table is private-use and is not exported, not installed
   to httpd target include/, etc. util.c was its sole consumer.

 * mod_log_forensic changed this and started consuming an include
   which was buried under server/ for its own devices. Questionable,
   but still a core module (changes to the core table less likely to be
   caught when reviewing the module, but the toggle is clearly labeled
   FORENSIC, so there's that. Note the module duplicates the entire
   table, but it's forensic/diagnostic so I don't care what such code
   and const static footprints look like.)

 * T_HTTP_TOKEN_STOP declared NUL did not stop a token. That
   is an idiotic assertion. I inverted it. Cursory examination of the code,
   I did not spend enough time studying side-effects of the end-of-string
   behaviors, nor did the many other reviewers, I looked at the immediate
   affected logic. All use of this array and it's value of element NUL are
   exercised in util.c, this is not like it was some obscure external citation.

 * I agree with you the entire thing is unclear because there is no entity
   called a TOKEN_STOP, although there are recognized separators,
   and non-token garbage entities, but the functions implicated never
   bothered to distinguish between any of this. T_HTTP_TOKEN is
   clearer and less ambiguous (and NUL does not belong in that set.)
   I'm about to propose that change and will VOTE DOWN any call
   to backport that change to 2.4, as...

 * This change is mutually agreed to be a no-op, and you've made
   clear the intent was to backport to a stable, maintenance branch
   which should see NO deltas which have no effect. For example,
   whitespace correction is absolutely prohibited on a release branch
   because the svn blame ... comprehension of the changes to the
   code is destroyed by a stupid endeavor to make things look nice.
   When whitespace changes are introduced, it is in order to port
   the minimal, identical changes previously applied to trunk, so that
   a functional patch can be applied cleanly from an svn merge.

So to convince me my veto is unwarranted, you need to basically
convince me that the array elts are too volatile to trust, including the
elt[0], or that we have a long tradition of getting this wrong (I suggest
we don't - this was an exception), that we are unable to maintain the
pairing util.c <> test_char.h. OR that we are about to export test_char.h
to a much less attentive audience as a public export, etc.