I abhor comments like these. It says that it needs to allocate 5 more bytes, and I can see that in the code, but it doesn't say why the magic number is 5. The comment explains the what but not the why. The commit message is equally as useless: https://github.com/DaveGamble/cJSON/commit/65541b900c740e1d5...
What needs to be fixed so that this "+5" can be removed? What happens if something changes and the magic number needs to be 6? Also, where's the unit test?
If you've debugged the problem to the point where you know +5 will be sufficient, you should know enough to fix the bug that required it in the first place.
It's not a bug. It's a public API and the buffer is an input from the caller. Nothing goes wrong if the buffer is smaller than it ought to be. The doc comment is just warning the caller they might need a larger buffer than they think they do.
I read the comment as saying there's another call which gives you a recommended buffer size, but the value returned isn't always sufficient. That sounds very much like a bug to me. It could be a tweet just doesn't give enough context to know what the real problem is.
You can certainly fix known knowns, but you can't fix unknown unknowns. Sometimes its safer to simply apply a rule that you have confidence will work everywhere than fix that may introduce unsuspecting problems.
I never said that you don't track down a root cause.
Once you identify a root cause, you can assess the scope. "allocate 5 bytes more than you need" is a pretty broad reaching fix. I'd be very hesitant to implement it without proper time to fix.
I'd be satisfied with that if I was a reviewer in the code review. I'd want that (and the proofs) explicitly mentioned in the comment, or at the very least in the commit message.
At first glance the example seems contrived to me. Assuming that the code is deterministic, wouldn't it be trivial to measure the memory requirement for a particular input?
If that's all the case, then, where's the proof? It might be "easy to prove," but it's probably not an "I can prove this trivially, on the fly" type of task.
I think the real resolution would be change the function that estimates the amount of memory needed would return it with the additional 5 added already.
That way you can trust the estimator to always tell you at most how much memory you'd need.
Based on the original message that did + 64 and that this revised to + 5, it might be that a float64 can be serialized to at most 13 ASCII characters (not true in general but might be true of cjson's serializer; I didn't check). The original buffer would only have 8 bytes since that's the width of the f64, so 5 more bytes are needed.
It seems like the commit also slightly regressed the comment. Going from 'floating point value is printed', which at least gives a hint as to why, and replaced it with 'inaccuracies when reserving memory', which is useless for future developers.
My guess is this: they had an off by one error but didn't know it. When he allocated an extra 5 bytes, the problem went away. Instead of fixing the problem, they made an assumption as to why the extra bytes made the error go away.
What needs to be fixed so that this "+5" can be removed? What happens if something changes and the magic number needs to be 6? Also, where's the unit test?