On Thu, Oct 04, 2001 at 06:51:33PM -0700, Justin Erenkrantz wrote:
> 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.
Huh? That's a bit of a stretch. If this is going to turn into a style
war, let it be spoken from the Apache Style Guide:
"Opening braces are given on the same lines as statements, or on the
following line at the start of a function definition."
> 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. =)
My patch changes no visible functionality. Part of the patch simply added
comments, almost *ALL* of it was to improve robustness and readability.
Minor style changes definately fit into this.
Mixing major logical changes with style changes is a bad thing, and should
be avoided if not merely for the sake of the patch reviewer.
> > [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. =)
Bzzzt, wrong! Those are not semantically equivalent statments. The !
operator treats the operand as a boolean, the == operator treats the
operands as the same type. Let me do some english translations:
if (!ctx->remaining) {
"If ctx->remaining is not true..."
if (ctx->remaining <= 0) {
"If ctx->remaining is less than or equal to zero..."
There is a huge semantic difference there, and it plays directly into the
readability of the code.
> 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.
"bytecodes"? Now who's the java weenie? *grin* I think you meant
instructions. Less typing is good, but so is more reading (beautiful
code gets more eyes in the OSS world, no?). :)
> 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.
I absolutely agree, but the problem stems from the fact that we are
trusting out input parameters to be valid. We can not guarantee that
other modules will behave, so why not be a little bit more conservative
in how we behave?
> 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
That would also be satisfactory. As long as we aren't making implicit
assumptions on our input.
-aaron
