From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Stefan Kangas Newsgroups: gmane.emacs.devel Subject: Re: Merging scratch/substitute-command-keys to master Date: Sat, 17 Oct 2020 23:35:31 +0000 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="39698"; mail-complaints-to="usenet@ciao.gmane.io" Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane-mx.org@gnu.org Sun Oct 18 01:36:42 2020 Return-path: Envelope-to: ged-emacs-devel@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1kTvkg-000AG5-Dx for ged-emacs-devel@m.gmane-mx.org; Sun, 18 Oct 2020 01:36:42 +0200 Original-Received: from localhost ([::1]:58720 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1kTvkf-0000rL-Gq for ged-emacs-devel@m.gmane-mx.org; Sat, 17 Oct 2020 19:36:41 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:54944) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1kTvjb-0008Ub-8K for emacs-devel@gnu.org; Sat, 17 Oct 2020 19:35:35 -0400 Original-Received: from mail-ej1-f46.google.com ([209.85.218.46]:45033) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1kTvjZ-0006zO-ED for emacs-devel@gnu.org; Sat, 17 Oct 2020 19:35:34 -0400 Original-Received: by mail-ej1-f46.google.com with SMTP id a3so8725312ejy.11 for ; Sat, 17 Oct 2020 16:35:32 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:in-reply-to:references:mime-version:date :message-id:subject:to:cc; bh=RbB+S/iS6QZl9EjhbmEptAtt+o+r4nqvnCOIaBjqMqg=; b=Bu+3j5LpUKUvDe7EbYg4mo4NN9qQ4DcZ6jepF9pKEUgO8lnFBAFgCsIXaGl/CSSG+P gldXYRV4iPwcKe0sF54Aq8QeaKmTQj4Cu/2VvT++bkKLSFPPIgjVyz0fJpKjVdwuJKrc HrJVz4RMqpI3iEZ7bIwHfJSsgxGjUH7CQO/kRFa9ERWYYJnq9X5s5Bz7HZDUBlzcgYHR Czwl1r2chEiMD81dG+m0tx+6ujhv7A3caZPc0q4kxX/sf3lOw1uSAJkpC3hIQ/hWDmOQ 8U0ezM0kF4uO3keqBnFnn6o+Ky3QiMI2SyBS1QvMUO3d2Ob+s7CFpt4wn8kL/A9GdPPo G8wQ== X-Gm-Message-State: AOAM532rSizuL1cIA36qQ3zhFD4+0KuB2EqCfzn0Nhx6hempIWs3mqT8 mp3H2moE8AKo8iL13B5rShN7nd8b3RbBaEu7SZA= X-Google-Smtp-Source: ABdhPJywRshR+xo8kWTeWgR7LhcnXVLOdz7+CNIBJSOPOYEZbjsJUJekt2URw9PDcq/vH8w/w6yCxI16LVNlQhCjR9Q= X-Received: by 2002:a17:906:3ed0:: with SMTP id d16mr10893651ejj.477.1602977731879; Sat, 17 Oct 2020 16:35:31 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Sat, 17 Oct 2020 23:35:31 +0000 In-Reply-To: Received-SPF: pass client-ip=209.85.218.46; envelope-from=stefankangas@gmail.com; helo=mail-ej1-f46.google.com X-detected-operating-system: by eggs.gnu.org: First seen = 2020/10/17 19:35:32 X-ACL-Warn: Detected OS = Linux 2.2.x-3.x [generic] [fuzzy] X-Spam_score_int: -13 X-Spam_score: -1.4 X-Spam_bar: - X-Spam_report: (-1.4 / 5.0 requ) BAYES_00=-1.9, FREEMAIL_FORGED_FROMDOMAIN=0.25, FREEMAIL_FROM=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.25, RCVD_IN_DNSWL_NONE=-0.0001, RCVD_IN_MSPIKE_H3=0.001, RCVD_IN_MSPIKE_WL=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.23 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-mx.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.io gmane.emacs.devel:257979 Archived-At: Stefan Monnier writes: > I don't have a clean baseline against which to run such tests, but I can > confirm that the speed is comparable Thanks, that information is very useful. > See comments below. Other than those details, LGTM, [...] > We generally do either of those or yet something else (e.g. rebase, > with or without cleaning up the history). > I like to just merge, personally. Thank you kindly for the review. I've fixed all your comments unless indicated otherwise below. I'm currently working on cleaning up the history of the branch and will push a new version of the branch, that I then plan to merge to the master branch. >> +;; (defun describe-keymap-or-char-table > [...] >> +;; (let* ((val (aref keymap idx)) > > `aref` will signal an error when applied to a keymap. > So your `keymap` variable doesn't hold a keymap, and so your function > name is misleading. Right. I probably picked up this terminological confusion from describe_vector, where the documentation says, a bit confusingly, that "KEYMAP_P is 1 if vector is known to be a keymap". But of course a vector is never a keymap, at least not according to `keymapp'. So the argument here should better be called `vector' (as is the case in the describe_vector function on which this is based). > Also this function should probably have a "help--" prefix because it > should stay as an internal detail. See the discussion on this name below. >> + { >> + msg = build_unibyte_string("Key translations"); > ^ ^^ > Lisp_Object SPC > > Your compiler will be just as happy with many such "small scope" vars as > with your larger scope var. > > Also, you might want to use `AUTO_STRING` here. (I fixed the first part here.) I tried using AUTO_STRING at first, but these strings are passed to Qdescribe_map_tree so that leads to segfault if there is a GC. While investigating that, I learned that AUTO_STRINGs are allocated on the stack (I guess without the proper reference counters?) and shouldn't be passed to Lisp code. >> @@ -2793,8 +2815,11 @@ DEFUN ("describe-buffer-bindings", Fdescribe_buffer_bindings, Sdescribe_buffer_b > > BTW, I see that `describe-buffer-bindings` is a natural next candidate > for ELispification ;-) Indeed! >> +DEFUN ("describe-keymap-or-char-table", Fdescribe_keymap_or_char_table, > [...] >> + CHECK_VECTOR_OR_CHAR_TABLE (vector); > > The code says VECTOR_OR_CHAR_TABLE, so your function's name might want > to do the same. Or maybe call it `describe-vector` since it seems to be > a thin wrapper around that function. The problem is that there is already a `describe-vector' with a different calling convention. So perhaps we should just go with `describe-vector-or-char-table' (clunky as it may be) if no one has a better proposal. Or maybe `help--describe-vector' is better? Or the iniquitous `help--describe-vector-or-char-table'? Thanks again for reviewing.