Re: wait_reading_process_ouput hangs in certain cases (w/ patches)

[Matthias Dahl]

Hello Eli...

Attached you will find the revised patches which now properly
set the number of bytes read.

Sorry for the delay, but real life intervened.

Turns out, things were _a lot_ easier than I thought since the
C standard actually does have well-defined rules for computation
on unsigned types, so that no overflow happens. And since it is
modular arithmetic, it perfectly fits our use-case. :-) Sometimes
the simplest solution is right in front of your eyes and you are
thinking way too complicated.

So sorry for the trouble. Had I known this earlier, I would not
have skipped that part -- even though I had the best intentions,
obviously.

On with the rest of your mail...

On 07/11/17 17:40, Eli Zaretskii wrote:

> If we want, we can make it EMACS_INT, which will give us as much as
> Emacs can gobble.

Thanks for the suggestion, I changed it to EMACS_UINT -- we need an
unsigned type.

> I had my
> share of writing code based on what I read in the function commentary
> and some general common sense and familiarity with the internals, only
> to find out later that they were incomplete or even prone to wrong
> interpretation.  I'd like to minimize such occurrences as much as I
> can.

I guess I was overly idealistic. Sorry for that.

Nevertheless, it would be nice to improve the situation and make sure
that future changes to the codebase also take the commentaries into
account... and put those themselves to the same high standard as the
implementation itself. Just my two cents...

> But that's exactly the problem: we _don't_have_ documented interfaces
> on the C level, at least not of the quality we would like to have.  Do
> you really think that the commentary at the beginning of
> wait_reading_process_output describes what the function does anywhere
> close to completeness?  Far from it.

I know. OTOH, wait_reading_process_output might be a bad example as it
really could benefit greatly from a refactor since it is way too big,
complex and unclear. And describing its function properly and concisely
could prove quite difficult.

But generally, I pretty much agree on all that you said in the rest of
your mail.

> I'd actually say that the commentary is incomplete because it doesn't
> say what that "positive" value means.  Returning an opaque value from
> a function is not useful, unless you can pass it later to the same
> function to get it to do something special.

I agree. It also leads the user to make assumptions about the value
that might or might not turn out true for all cases... without properly
reading the code.

Thanks again for all your great feedback and review. I hope we now have
something that can be applied to the master branch. If anything comes up
in terms of bugs or problems that might be related, please poke me if I
miss it on the list and I will (try to) fix it.

Have a nice weekend,
Matthias

-- 
Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu

0001-Add-process-output-read-accounting.patch
Description: Text Data

0002-src-process.c-wait_reading_process_output-Fix-wait_p.patch
Description: Text Data