search

eddic / fastcgipp

fastcgi++: A C++ FastCGI and Web development platform:
https://fastcgipp.isatec.ca
GNU Lesser General Public License v3.0
310 stars 94 forks source link

Fix dump istream #46

Closed Erroneous1 closed 6 years ago

Erroneous1 commented 6 years ago

gcount() sometimes will be < maxContentLength before the end of file

eddic commented 6 years ago

Hmmmm. Well then. I can not for the life of me understand why I had it like that. You've looked at the code more recently than me, does that line make any sense?

Erroneous1 commented 6 years ago

Actually it might work fine if you change < to ==. I reviewed the documentation and I was wrong about gcount possibly returning less than what was requested after read. It clearly states that read will not return until count characters has been read or eof. I can amend PR tomorrow to fix it for both dump functions.

eddic commented 6 years ago

Yeah, the only thing I'm worried about with your original patch is that the while(stream) will only return an eof related false if stream.read() tried to read at minimum 1+ the eof point (as far as I know). If the file was exactly 0xffff bytes, I believe it will loop again and ultimately send a zero length record... no? For this reason I would agree that changing < to == is likely the best option.

eddic commented 6 years ago

You know what. Now that I think about it == is broken too. It's all broken. Check out 364985f9. I believe this should fix it. Does this sort it?

Erroneous1 commented 6 years ago

Much better, thanks!

Erroneous1 commented 6 years ago

Ok I found another test case and it isn't working anymore... I'm working on a solution but had a question. Is there any reason you don't use constexpr except in BigEndian's constructor? I'd like to start putting it in places if that's ok.

eddic commented 6 years ago

Hell yeah, if you find spots that can be constexpr then absolutely do it up. There isn't any reason at all that I didn't do it; should have but didn't. Just keep the constexprization into it's own commit and I'll pull it for sure.

Erroneous1 commented 6 years ago

BTW, is there a .clang-format I should use? I use a different style normally so being able to auto format my changes before making a PR would be nice.