Created attachment 2402 [details] gtk2-session-remove-duplicate-input-checks-from-session_w.patch The caller already did the same checks.
Created attachment 2403 [details] master-session-remove-duplicate-input-checks-from-session_w.patch
This is also true of session_write_data()
"The caller already did the same checks." I believe it is normal good practice for a function to check its own inputs rather than rely on callers passing exactly the right values. So if anything it should be the checks in the caller that are removed, but maybe they are needed for other reasons, I haven't checked.
I agree with comment #3.
(In reply to djh from comment #3) I disagree with that. There is a reason strlen(NULL) crashes. Drepper has a good writing somewhere, which essentially says "always know the state of your data". Which in this case means the caller is responsible. It is true that during development such checks will be sprinkled all over the code, especially when the API evolves to something different than initially envisioned. Once the API is done, and hopefully properly documented, these superfluous checks need to be removed. They just distract the reader. Like in this case. One may argue that in this specific case the actual checks can remain in session_write_buf because the function uses these values, and should be removed from the caller instead, because it does nothing with them. Either way is fine with me. One may argue further that checks in both functions could be removed. The checks are always true. But it needs a separate audit/patch/bug, if session_write_msg_cb is really called again if it returned FALSE.
If you provide a reference to whatever Drepper wrote, I'll be happy to read it. Otherwise it's just what you claim. I would argue that "always know the state of your data" is in full agreement with what I wrote. Generally it is worth being able to identify where a fault has occurred, even in the event that code changes. So if somebody writes some new code that calls the function then having the checks in place means that an error will be detected. Such checks are more important than optimising speed or space in most cases. Assertions can sometimes be used to achieve the best on both axes. I don't find boiler plate code like this distracting in general.