I found and fixed a bug in PHP's standard library(miguelxpn.com) |
I found and fixed a bug in PHP's standard library(miguelxpn.com) |
https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794...
They also fixed the whitespace handling that let something like "RandomHeader: hello host:8080" mistakenly set the flag.
https://github.com/php/php-src/commit/56cdbe63c24b86c2f1d60b...
Otherwise, it looks like it will call strstr() one extra time, even though you may have already determined that the specific header is present.
[1] https://github.com/php/php-src/commit/3d9c02364db62a6d8e2794...
while ((s = strstr(s, "host:"))) {
if (s == t || *(s-1) == '\r' || *(s-1) == '\n' ||
*(s-1) == '\t' || *(s-1) == ' ') {
have_header |= HTTP_HEADER_HOST;
}
s++;
}
This s++ could be s + sizeof "host:" - 1.Reason being: if you have just found "host:" at address s, you will not find another one at s+1, s+2, s+3, s+4 or s+4; "host:" supports no overlapped matches.
Actually since, we are really looking for "host:" preceded by whitespace, we can skip by just sizeof "host", (five bytes). Because if s points at "host:host:", the second "host:" is not of interest; it is not preceded by a whitespace character; we won't be setting the HTTP_HEADER_HOST bit for that one.
Also, once we set the HTTP_HEADER_HOST bit, we can break out of the loop; there is no value in continuing it. The point of the loop is not to have a false negative: not to stop on a false match on "host:" which doesn't meet the HTTP_HEADER_HOST conditions, and thereby fail to find the real one later in the string. If we find the real one, we are done.
By the way, this test shows how verbose of a language C is:
*(s-1) == '\r' || *(s-1) == '\n' ||
*(s-1) == '\t' || *(s-1) == ' '
If we could use Javascript, look how nice it would be: strchr("\r\n\t ", s[-1])[1] https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4....
There is no other RFC compliant way to end headers.
Even aside from DRY, extracting a block of code to a function or method gives you an opportunity to name the block of code, which often significantly clarifies the intent of the code so, I’ve always tried to error on the side of overly DRY
When I first took over maintaining Red Moon, I was a very new dev and went a little DRY crazy. In particular, there's a state machine for the different filter states (running, paused, stopped, etc), that had a bunch of classes (one per state) that had a lot of overlap. I pulled some common behaviors out into a supertype that the classes with those behaviors now inherited from.
Except, it turns out some of those states ought to act differently (automatic pauses in secure apps should disable the overlay but not restore the backlight; manual pauses should do both), and now it's going to be extra work untangling the various states and pause logic. If I'd left the state machine (amongst other bits) as it was, this feature/bugfix would be implemented already. Also, the current state of things (pun intended) is a bit less readable, in my opinion.
For instance, one piece of code is supposed to do the same thing as another, but in context it caused a side effect that some other part of the code was inadvertently relying on.
Sometimes it's better to have just a few separate, differently shaped pegs, than one flexible peg that fits any hole but is so complex that it might introduce 10 new bugs along the way.
In many cases, the opportunity for DRY was misleading as the code was only superficially similar, so the implementation became over-complicated to handle various corner cases that otherwise wouldn’t have existed.
Here's the file I was referencing (only 100 lines!): https://github.com/LibreShift/red-moon/blob/master/app/src/m...
Here's the issue I've been putting off fixing because of it: https://github.com/LibreShift/red-moon/issues/208
The problem is that right now there is one PAUSE state, which is triggered by both manual pauses (brightness should be restored) and automatic pauses (in secure apps; brightness should not be restored). The problem wasn't exactly DRY, it's the misapplication: I merged bits that had identical code, but turned out not to have identical purpose.
And, for posterity, here's what it used to look like way back (DRYing was not the only change, I also split out notification stuff, so it's not a 1:1 length comparison): https://github.com/LibreShift/red-moon/blob/11ae916955ff8c36...
edit: and here's the original file, back mostly before I touched it at all: https://github.com/LibreShift/red-moon/blob/ed2ec4fd1c68611d...
Nobody here is saying DRY is bad, just that it's not universally the right thing to do without consideration.
Embarrassing to admit, but until now I didn't think in this perspective - and on hindsight it should be obvious, sometimes code might look duplicate, but is not
Great fun.