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.bugs Subject: bug#42168: 26.1; cperl-mode: Bad interpretation of $a++ / $b Date: Thu, 13 Aug 2020 01:17:59 -0700 Message-ID: References: Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="21657"; mail-complaints-to="usenet@ciao.gmane.io" To: Harald =?UTF-8?Q?J=C3=B6rg?= , 42168@debbugs.gnu.org Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Aug 13 10:19:51 2020 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1k68Sk-0005TR-8Y for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 13 Aug 2020 10:19:50 +0200 Original-Received: from localhost ([::1]:56880 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1k68Sj-0006Yj-82 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 13 Aug 2020 04:19:49 -0400 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]:50136) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1k68Ry-0005hb-5k for bug-gnu-emacs@gnu.org; Thu, 13 Aug 2020 04:19:02 -0400 Original-Received: from debbugs.gnu.org ([209.51.188.43]:35169) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1k68Rx-00075g-TH for bug-gnu-emacs@gnu.org; Thu, 13 Aug 2020 04:19:01 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1k68Rx-000358-Pn for bug-gnu-emacs@gnu.org; Thu, 13 Aug 2020 04:19:01 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Stefan Kangas Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 13 Aug 2020 08:19:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 42168 X-GNU-PR-Package: emacs Original-Received: via spool by 42168-submit@debbugs.gnu.org id=B42168.159730668711777 (code B ref 42168); Thu, 13 Aug 2020 08:19:01 +0000 Original-Received: (at 42168) by debbugs.gnu.org; 13 Aug 2020 08:18:07 +0000 Original-Received: from localhost ([127.0.0.1]:46715 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k68R4-00033s-TZ for submit@debbugs.gnu.org; Thu, 13 Aug 2020 04:18:07 -0400 Original-Received: from mail-yb1-f177.google.com ([209.85.219.177]:44479) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1k68R3-00033N-KX for 42168@debbugs.gnu.org; Thu, 13 Aug 2020 04:18:06 -0400 Original-Received: by mail-yb1-f177.google.com with SMTP id i10so2847859ybt.11 for <42168@debbugs.gnu.org>; Thu, 13 Aug 2020 01:18:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:in-reply-to:references:mime-version:date:message-id:subject:to :content-transfer-encoding; bh=vkmhd6MtubjkKkh7YDKmhR+9ZwtzidNVgOuSx9u2pPI=; b=pBwTS8MD1nsNFiJiy4Zu6MD10oQi7OcbSt9vyb5MsP94oAL1Xwlp0pzOobioPhyKZB EBE719biSKd4veL2QeJCbZow+FPMmrxfRwGIGS+i5CL1tIko3p6FKmDLN+qXjYNASZfH f4IMn+4vO4QQsgyNBcgO4JhBRU5T+ytoLwVt/2LDVq4Qv3/do0EfZNcdFQLn3rXBa1JU /k/RcYsyCe8N73OCfw4qjEBn9dF5p1WcfcFXe48A8MVARHouTGc65V+aQGvikGJLv0Vf 2AEA3K1P0m0mV69ODieDtDaQ8R3LikUfBfQXGRcECDNNF3rVzWYDcVwVMu96tgbNiFZW PNdA== 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:content-transfer-encoding; bh=vkmhd6MtubjkKkh7YDKmhR+9ZwtzidNVgOuSx9u2pPI=; b=W1yGBIy4fzdCDXYWZK8oOZwBwyGrhl5BztDnjeT0BwkWqdR4Rb74bEbdCQDIR4Wpis 1/7mqKIOdRNbZaEmDFtpY8GCXee4vf4okfQ4O5n5C8PDdj9WZAMkIhxu/cOKb0X5HALL MajB6OI1pv+NaPMb7/Ksc0ec8/qOZEaqdLXhnBNJh/bn3Ph3HNczoTTSi7V6wBZj1beh W+uqPa/ErPy94eeE7bD1CVxYo04B/crVekmkR44iiclesORVEyj0RmrjTiP02p0kA0DK xMt6usKRr5MwcAPOuMRPH4TuH8gWz7s0EriEpHA8BxqgxVo5DtwQIxEkjKHLH2zcvMq5 Hngw== X-Gm-Message-State: AOAM530fIZ71c7d9RojlH770OHyaxmySF8ADIgUCUGnHju3qCSnahX0Z oMSHrm0Ucg3UdjJkDgXYAv4Nq9jHaDpBmBbQMhQ= X-Google-Smtp-Source: ABdhPJzAQ94dghdFWO8xNY/gJ5KRQ2PkEWVDbPlmkjhD73Djen9Us9PIPrprWaUNQSUq22yvs0zMc8+HV6fuS9WlQDY= X-Received: by 2002:a25:7007:: with SMTP id l7mr4770311ybc.85.1597306680021; Thu, 13 Aug 2020 01:18:00 -0700 (PDT) Original-Received: from 753933720722 named unknown by gmailapi.google.com with HTTPREST; Thu, 13 Aug 2020 01:17:59 -0700 In-Reply-To: X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.io gmane.emacs.bugs:184958 Archived-At: Hi Harald, Please see my comments on your patch below. Harald J=C3=B6rg writes: > diff --git a/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el = b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el > new file mode 100644 > index 0000000000..4148db036c > --- /dev/null > +++ b/test/lisp/progmodes/cperl-mode/cperl-fontification-tests.el Great initiative to also write tests for this bug. For now, I think you should simply place the file directly at: lisp/progmodes/cperl-mode-tests.el We could always break it up into several files later when it gets bigger and there is a need to do so. > @@ -0,0 +1,55 @@ > +;;; cperl-fontification-tests.el --- Test fontification in cperl-mode -*= - lexical-binding: t -*- > + > +;; Copyright (C) 2020-2020 ...to be decided ... All files in GNU Emacs should be following this template: ;; Copyright (C) 2020 Free Software Foundation, Inc. (Note that you only need a range if there's more than one year.) > +;; Author: Harald J=C3=B6rg > +;; Maintainer: Harald J=C3=B6rg > +;; Keywords: internal > +;; Human-Keywords: internal Nit: I think we can skip Human-Keywords here. > +;; Homepage: https://github.com/HaraldJoerg/cperl-mode > + > +;;; Commentary: > + > +;; This is a collection of Tests for the fontification of CPerl-mode. Typo: "tests" shouldn't have a capital letter. > +;; The primary target is to verify that the refactoring we're doing > +;; right now (May 2020 - ...) doesn't change any behavior, or does the > +;; right thing in cases where new fontification rules are enabled. I think these three lines here could be removed, maybe? It seems to me that they are describing the purpose of all tests, namely to stop regressions from happening. > +;; Run these tests interactively: > +;; (ert-run-tests-interactively '(tag :fontification)) Nit: Missing this header here: ;;; Code: > + > +(defun cperl-test-face (text regexp) > + "Returns the face of the first character matched by REGEXP in TEXT." > + (interactive) > + (with-temp-buffer > + (let ((cperl-hairy nil) > + (cperl-font-lock nil)) ;; Does this matter? Does it matter? Not sure, what happens if you remove it? :-) > + (insert text) > + (cperl-mode) > + (font-lock-fontify-buffer) > + (goto-char (point-min)) > + (re-search-forward regexp) > + (message "%s" (match-string 0)) > + (get-text-property (match-beginning 0) 'face)))) It is good practice to remove calls to 'message' in the tests, since they mostly make the tests more noisy. If it's really useful during development of tests, you could comment it out or make it dependent upon a new variable like > +(ert-deftest jrockway-issue-45 () Is probably better named as: cperl-mode-test-bug-42168 to refer back to our own bug in the name. We already have the link to jrockway in the doc string, which is useful if one needs to dig deeper. > + "Verify that '/' is a division after ++ or --, not a regexp. > +Reported in https://github.com/jrockway/cperl-mode/issues/45. > +If seen as regular expression, then the slash is displayed using > +font-lock-constant-face. If seen as a division, then it doesn't > +have a face property." > + :tags '(:fontification) > + ;; The next two Perl expressions have divisions. Perl "punctuation" > + ;; operators don't get a face. The comment at the end of line > + ;; prevents cperl-mode from tripping over "End of =E2=80=98/ ... /=E2= =80=99 > + ;; string/RE not found" if it gets it wrong Do we need the comment at the end of the below code fragments with your patch as well? If not, couldn't we just remove them? I think that would make the intention a little bit clearer, maybe. > + (let ((code "{ $a++ / $b } # /")) > + (should (equal (cperl-test-face code "/" ) nil))) > + (let ((code "{ $a-- / $b } # /")) > + (should (equal (cperl-test-face code "/" ) nil))) > + ;; The next two Perl expressions have regular expressions. The > + ;; delimiter of a RE is fontified with font-lock-constant-face. > + (let ((code "{ $a+ / $b } # /")) > + (should (equal (cperl-test-face code "/" ) font-lock-constant-face))= ) > + (let ((code "{ $a- / $b } # /")) > + (should (equal (cperl-test-face code "/" ) font-lock-constant-face))= )) The rest looks good to me. Best regards, Stefan Kangas