@rjvbb Please do not use "Differential Revision: https://phabricator.kde.org/Dxyz" in commits of code which is not that very review. Phabricator interprets that line not as "please add a comment to that review discussion", but as "this commit is the patch which is reviewed there".

BTW, you skipped the request for adding a comment about the uncertainness what a valid values is.

"this commit is the patch which is reviewed there".

Not entirely wrong here, is it? The DR was already closed and the reference to the initial commit is still there, ;)

BTW, you skipped the request for adding a comment about the uncertainness what a valid values is.

I did what Kevin asked me, and indeed forgot about adding a comment. Should I add one in an additional commit? I'm not certain it's that crucial; I think that the assert is sufficiently clear on what is currently considered a valid return value. The only condition is that value is positive unless the stream is NULL, in which case the value should be 0. There are also the references in the commit message.

In R32:9b581d93bbc5794a197287ee0923607a57e90113#3696379, @rjvbb wrote:

"this commit is the patch which is reviewed there".

Not entirely wrong here, is it? The DR was already closed and the reference to the initial commit is still there, ;)

The initial commit though is the main patch here, and hidden by default by the follow-up fix. While the review is closed, it is not removed from the database for a reason, as future code readers following the link in the commit message on the original patch come here to read any potential discussions on the original patch. First having to find the actual patch that got comitted under the stack of any follow-up fixes is not helpful.

Please avoid this in the future, existing review requests can be mentioned by a plain Dxyz in the commit message or by using direct links to the actual comment the fix is referring to.

BTW, you skipped the request for adding a comment about the uncertainness what a valid values is.

I did what Kevin asked me, and indeed forgot about adding a comment. Should I add one in an additional commit? I'm not certain it's that crucial; I think that the assert is sufficiently clear on what is currently considered a valid return value. The only condition is that value is positive unless the stream is NULL, in which case the value should be 0. There are also the references in the commit message.

The context of that code is clear to us now, as we invested time and brain power into it. A future reader (even your older self) of the code that got committed does not have that context (any more), they will need to dig up all the things again, where a simple comment in the code would spare that effort to good degrees. This is not throw-away code, so let's be nice to future maintainers.
Kevin already pushed some comment, so this can be checked off.