From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Matthias Dahl Newsgroups: gmane.emacs.devel Subject: Re: wait_reading_process_ouput hangs in certain cases (w/ patches) Date: Wed, 22 Nov 2017 15:33:18 +0100 Message-ID: <07fdb378-0a42-4980-5614-38afb5ae7749@binary-island.eu> References: <83tvy7s6wi.fsf@gnu.org> <83inemrqid.fsf@gnu.org> <398f8d17-b727-d5d6-4a31-772448c7ca0d@binary-island.eu> <56e722a6-95a4-0e42-185c-f26845d4f4bf@binary-island.eu> <21237e45-a353-92f9-01ec-7b51640d2031@cs.ucla.edu> <83vaickfu2.fsf@gnu.org> <83tvxwkexg.fsf@gnu.org> <03261534-6bf5-1a5d-915f-d3c55aaa35e9@binary-island.eu> <206ebefa-7583-f049-140c-c8fd041b0719@cs.ucla.edu> <709614e8-1937-07c1-f554-b453ed4f3d4a@binary-island.eu> <7550438b-9fd4-d374-e571-8bb16456cad5@cs.ucla.edu> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Trace: blaine.gmane.org 1511361254 3902 195.159.176.226 (22 Nov 2017 14:34:14 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 22 Nov 2017 14:34:14 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 Cc: emacs-devel@gnu.org To: Paul Eggert , Eli Zaretskii Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed Nov 22 15:34:08 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1eHW6G-0000MP-Bo for ged-emacs-devel@m.gmane.org; Wed, 22 Nov 2017 15:34:04 +0100 Original-Received: from localhost ([::1]:39831 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHW6N-0003uy-EY for ged-emacs-devel@m.gmane.org; Wed, 22 Nov 2017 09:34:11 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:42553) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eHW5i-0003um-Lr for emacs-devel@gnu.org; Wed, 22 Nov 2017 09:33:31 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eHW5c-0000Ps-Tb for emacs-devel@gnu.org; Wed, 22 Nov 2017 09:33:30 -0500 Original-Received: from ud19.udmedia.de ([194.117.254.59]:39666 helo=mail.ud19.udmedia.de) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eHW5c-0000Mb-H6 for emacs-devel@gnu.org; Wed, 22 Nov 2017 09:33:24 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=simple; d=binary-island.eu; h= subject:to:cc:references:from:message-id:date:mime-version :in-reply-to:content-type:content-transfer-encoding; s=k1; bh=L/ ju7I8edwBg6cKYbLg9Tl1puTJh5h+hE5VIFe3Y/Jw=; b=y+ACR+Mj7+khTaVAIN iv0L0zGSdpKZsi88gFHQkGrL/1gcCNWPF/aoIY5iGbCb6AKRn5faYkmsOJVbXS9I wpirjOdoXmIh/iLhBzXfE6gPwtiaDLRLUnBNflE3aOFfk0JH8yowzBs7MsP+AK5b Hrh7qMhJp71LvDix0NnRPKt/k= Original-Received: (qmail 31328 invoked from network); 22 Nov 2017 15:33:21 +0100 Original-Received: from unknown (HELO ?IPv6:2a02:810b:c540:234:36aa:25b9:ca8f:d05f?) (ud19?126p1@2a02:810b:c540:234:36aa:25b9:ca8f:d05f) by mail.ud19.udmedia.de with ESMTPSA (ECDHE-RSA-AES128-GCM-SHA256 encrypted, authenticated); 22 Nov 2017 15:33:21 +0100 Openpgp: id=1E87ADA02EFE759EFC20B2D1042F47D273AA780C In-Reply-To: <7550438b-9fd4-d374-e571-8bb16456cad5@cs.ucla.edu> Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] [fuzzy] X-Received-From: 194.117.254.59 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:220355 Archived-At: Hello Paul... On 22/11/17 09:55, Paul Eggert wrote: > This doesn't look right, as nbytes might be negative (indicating an error). You are absolutely right. I initially pushed it one line up before the carryover is added but at a late call decided to put it a bit further up for clarity and missed the conditional below somehow (turning blind!?). Sorry about that. I will pay more attention in the future... > Please start with a summary line that doesn't contain so much detail. > <=50 chars is good. No trailing period. "Fix > wait_reading_process_output_hang", perhaps. Ok. Thanks for clearing that up. It was hard to figure out what style is requested for Emacs. > Kind of a long name, no? Perhaps make it shorter, so that you can write > something like this: > >    uintmax_t nbytes_read0 = wait_proc ? wait_proc->infd_num_bytes_read : 0; > > Even better, shorten the member name too: "nbytes_read" is easier to > read than "infd_num_bytes_read". Well... I will have to think about that one. I am a firm proponent of descriptive names that convey exactly what a function or variable is being used for -- even at the expense of the length of that name. nbytes_read is, imho, too generic -- especially given the huge context of wait_..., so a good descriptive name helps. Like I said, I will think about something that is shorter -- but coming up with this name was not as easy as it may seem because I naturally aimed at something shorter as well. > Please limit the program to 80 columns. Ok, will do. > Space before "(" in function calls. Argh, sorry, missed that again. I am not used to that style. > It's annoying that the code (and the comment!) is duplicated. I absolutely agree but quite honestly I was convinced I would get negative feedback if I did put it in a function of its own, given how the rest of wait_... looked and in general. So I went with the duplicated code for exactly that reason. Quite happy to change that. I will have updated patches available later this week. Thanks for your great feedback, again. So long, Matthias -- Dipl.-Inf. (FH) Matthias Dahl | Software Engineer | binary-island.eu