From mboxrd@z Thu Jan 1 00:00:00 1970 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=0.3 required=5.0 tests=MAILING_LIST_MULTI, RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,RDNS_NONE,UNPARSEABLE_RELAY autolearn=no autolearn_force=no version=3.4.4 Received: from authenticated user by zero.zsh.org with local id 1kiu5r-000HHR-SK; Sat, 28 Nov 2020 06:52:27 +0000 Authentication-Results: zsh.org; iprev=pass (relay6-d.mail.gandi.net) smtp.remote-ip=217.70.183.198; dmarc=none header.from=chazelas.org; arc=none Received: from relay6-d.mail.gandi.net ([217.70.183.198]:57845) by zero.zsh.org with esmtps (TLS1.2:ECDHE-RSA-AES256-GCM-SHA384:256) id 1kiu5T-000H2k-Dd; Sat, 28 Nov 2020 06:52:04 +0000 X-Originating-IP: 94.10.124.211 Received: from chazelas.org (unknown [94.10.124.211]) (Authenticated sender: stephane@chazelas.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 082CEC0005; Sat, 28 Nov 2020 06:52:01 +0000 (UTC) Date: Sat, 28 Nov 2020 06:52:01 +0000 From: Stephane Chazelas To: Daniel Shahaf Cc: zsh-workers@zsh.org Subject: zsh coding style (was about a limit patch review) Message-ID: <20201128065201.54ngcgcyklyfggm6@chazelas.org> Mail-Followup-To: Daniel Shahaf , zsh-workers@zsh.org References: <20201123214942.hi2rx7n3jk25ucmd@chazelas.org> <74327-1606347813.918593@HxCz.NV4p.AwzH> <20201126205819.dbncs24ppnw3pdny@chazelas.org> <20201127163949.GD26720@tarpaulin.shahaf.local2> <20201127201342.t4uthqbfwuhzgevz@chazelas.org> <808eb7ef-fe44-474a-ba85-6144b95b16c2@www.fastmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <808eb7ef-fe44-474a-ba85-6144b95b16c2@www.fastmail.com> X-Seq: 47688 Archived-At: X-Loop: zsh-workers@zsh.org Errors-To: zsh-workers-owner@zsh.org Precedence: list Precedence: bulk Sender: zsh-workers-request@zsh.org X-no-archive: yes List-Id: List-Help: List-Subscribe: List-Unsubscribe: List-Post: List-Owner: List-Archive: Archived-At: 2020-11-27 20:36:40 +0000, Daniel Shahaf: [...] > Of course, DRY; but still, notice how one of the ints is actually > a boolean, so the formal type information doesn't convey everything. [...] > > + * Arguments: > > + * - s: string to parse > > + * - lim: resource being limited (from which will derive the type and unit) > > + * - ulimit: to specify whether we're being called by ulimit or not. > > + * For ulimit, suffix-less limits are multiplied by the limit's > > + * unit. > > Suggest to add the word "boolean" to the description of this parameter. > > Since you've already gone and described it in prose, I'd consider to > also rename it to something like > «multiply_suffixless_limits_by_implied_unit_p» (trailing _p for > "predicate", i.e., signifies the variable is a boolean), to avoid caller–callee coupling. > > > + * - err: (return value) error to be returned if any. If a non-NULL value is > > + * stored there, zstrtorlimt failed and the return value is > > + * irrelevant (though will be 0). > > You don't actually say anywhere what the return value will be when there > _isn't_ an error. [...] Believe it or not, I had "(boolean)" and a "Returns:..." initially, and took them out before submitting as they would have been out of character among the other functions in that file (and the rest of the code from a cursory look) which are rather minimalist and avoid stating the obvious. This function description is already by far the least dry and probably the most RY in that file. To the point that I'm wondering if you're pulling my leg for being overly wordy here. About the _p suffix, the only usage of it in the code (beside has_p) I could find for it was save_params(Estate state, Wordcode pc, LinkList *restore_p, LinkList *remove_p) in exec.c, but that "p" looks like it's for "pointer" (as in return value, though could also be "param"), not "predicate". I've never heard of _p for predicate before, but then again I've not done much C development lately. I'm all for having a coding style. Even a template for function header could be useful. That would probably help make the code more legible (though in general, I do find zsh's code pretty legible, orders of magnitude more so than ksh93's for instance) I did read Etc/zsh-development-guide before submitting my first patch. The relevant bit there is: } * Function declarations must look like this: } } /**/ } int } foo(char *s, char **p) } { } function body } } } } There must be an empty line, a line with "/**/", a line with the } type of the function, and finally the name of the function with typed } arguments. These lines must not be indented. The script generating } function prototypes and the ansi2knr program depend on this format. Maybe that document could be updated to say more precisely how new code should look like if we're to enforce a new coding style. -- Stephane