On Thu, Oct 04, 2001 at 01:53:12PM -0700, Aaron Bannert wrote:
> On Thu, Oct 04, 2001 at 01:29:26PM -0700, Ryan Bloom wrote:
> > > > - if (!ctx->remaining)
> > > > - {
> > > > + if (ctx->remaining <= 0) {
> > >
> > > No. =) Java weenie.
> >
> > Huh? How is this him being a Java weenie? Aaron has changed a check
> > for !0 to a check for negative or 0. Those two checks are not equivalent.
> > I'm not sure if this is a valid change or not yet, but I am wondering what the
> > comment is about.
My comment about Aaron being a Java weenie is that he changed the
brace to fit the recommended Java style. At eBuilt, they wanted
us to place braces on the same line all the time - we had long
discussions about this. I refused. =) And, I think Aaron did
too at the time. He's gone over to the darkside. Nothing wrong
with that.
As an aside, I'd rather not see any style changes be in the same
commit/patch as something that changes bytecode - unless enough
of the function is changed to warrant the rewrite. I've been
told many times not to change style in a patch - and for the most
part, I try not to do that anymore. I'd just like to reinforce
that behavior in others. =)
> [To Justin]
>
> Even if we were checking for zero/non-zero, I would want it to be
>
> (ctx->remaining ==/!= 0)
>
> We are not checking the boolean status of the variable, we are checking
> the integer status. Shorthand does not equate to fewer instructions
> and sometimes defeats readability.
No, I think that if you are not familiar that the C construct
(ctx->remaining) is equivalent to (ctx->remaining != 0) than I
don't really have any sympathy for you. IMHO, we're not here to
teach people C. =)
I agree that it doesn't equate to any more or less bytecodes, but
it *does* equate to less typing for me - which is what I'm all
about now that my hands hurt so much.
As I said before, I think that if we have ctx->remaining < 0,
then someone did something horribly wrong. And, I think if
someone didn't pay attention to r->remaining, it would be
possible to enter this state.
So, I think we should fix that problem (so that we can't read
past the end of the body) - not hide it by checking for negative
values. So, I think we should check ctx->remaining < readbytes
before we call ap_get_brigade in ap_http_filter. If ctx->remaining
is less than readbytes, only read in ctx->remaining. How does
that sound? -- justin
