From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: =?iso-8859-1?Q?Jo=E3o_T=E1vora?= Newsgroups: gmane.emacs.devel Subject: Re: Apropos 54f297904e0c: Temporarily comment out CC Mode from tests which are incompatible with it. Date: Fri, 18 Jan 2019 19:55:20 +0000 Message-ID: References: <20190117164350.GA18314@ACM> <20190118175437.GA4095@ACM> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: quoted-printable X-Trace: blaine.gmane.org 1547841274 27762 195.159.176.226 (18 Jan 2019 19:54:34 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 18 Jan 2019 19:54:34 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.0.50 (windows-nt) Cc: emacs-devel To: Alan Mackenzie Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Fri Jan 18 20:54:30 2019 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([209.51.188.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1gkaDm-00077s-0j for ged-emacs-devel@m.gmane.org; Fri, 18 Jan 2019 20:54:30 +0100 Original-Received: from localhost ([127.0.0.1]:45853 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkaFt-0001wf-9i for ged-emacs-devel@m.gmane.org; Fri, 18 Jan 2019 14:56:41 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:33341) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1gkaF5-0001wT-DE for emacs-devel@gnu.org; Fri, 18 Jan 2019 14:55:53 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1gkaF2-000077-EK for emacs-devel@gnu.org; Fri, 18 Jan 2019 14:55:51 -0500 Original-Received: from mail-wr1-x42f.google.com ([2a00:1450:4864:20::42f]:32836) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1gkaF2-0007RM-41 for emacs-devel@gnu.org; Fri, 18 Jan 2019 14:55:48 -0500 Original-Received: by mail-wr1-x42f.google.com with SMTP id c14so16563150wrr.0 for ; Fri, 18 Jan 2019 11:55:26 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version:content-transfer-encoding; bh=Ty1k83B3qDZVexz7INnvJoxRQQnBKxPU2dYC0715opk=; b=fkIju4mA4NwQ7Pkb9BwG+ynGuCBI7Jn25eUhCrJ8M5Eh/L/XYeL/X5J4HAAIC8zTAy 8kXVWTgTrAFJwlfAwTVbz/V7B/xUK06WTUCVuasvlSYgI8VKM20KAdxFF8K+v5h/KXit 6vjeNjLhw+wNX1eh/UanxvyqrNADF5tbPttb74Fqg2ZDV8wygeFRV7LXuf1vL2OiEzjw +z2nvFYwq6ZObx1coyIwpEcgAYe+O84QBIzJEKXdK6j0goTptPaapEdhUMfFWoF5fECX 4OOnAdPXQcRDus4ftqidbGX8rmQx+fH/+CRJL8zzAjrCoC1H23okGTRD6EESDReD4JaY 4Esw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-transfer-encoding; bh=Ty1k83B3qDZVexz7INnvJoxRQQnBKxPU2dYC0715opk=; b=gajLwCnC1UGMj1eQd7Rsc+GFReUC2Y88EWyf2L82JR3SzIOrOvmJOQtajW6q64Dox1 CZ2YJeekuLOrPaW/g6WHpErPfLtKA5X70CNgUmfw8ogW9ikivYZX+MDd2NPcJ9LAJHPq /ujQw5Qjgr2m9PsEVsuq5kvRQWrLCu6v2vY0cRrBJa2X8dark5MuqzlKWQpo5XyAavud nHoA0ScOXaMF6gUreue35Ge1JKkCB+JylLkuj/AGNQNO5j5WR6SWFhoNAEYewpaoCn3p cHd/ES2VQDWdCP45B2s/aXXCGLMdf2ZffEAF1IhyY4Rv2uFZNNOiZ3t4VsXQoA0cAes0 3sRg== X-Gm-Message-State: AJcUukeH0mc5A42XQUmIW2V7SPaNUcKsuzAA8ZjP+Js22CVZPscmd/oB 41+x75IgkAaDcA/lWNpejk6YcY1L X-Google-Smtp-Source: ALg8bN4NTCq8aEByJMoyzJcQDotkXziMmfbl2EmvTqNbxxmSlyGdu/41gmxuX/sV0I+JfOTkWQqfKg== X-Received: by 2002:adf:9168:: with SMTP id j95mr17572549wrj.217.1547841323453; Fri, 18 Jan 2019 11:55:23 -0800 (PST) Original-Received: from GONDOMAR.yourcompany.com (mail3.siscog.pt. [195.23.29.18]) by smtp.gmail.com with ESMTPSA id q3sm89049848wrn.84.2019.01.18.11.55.21 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 18 Jan 2019 11:55:22 -0800 (PST) In-Reply-To: <20190118175437.GA4095@ACM> (Alan Mackenzie's message of "Fri, 18 Jan 2019 17:54:37 +0000") X-Antivirus: AVG (VPS 190118-2, 18-01-2019), Outbound message X-Antivirus-Status: Clean X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2a00:1450:4864:20::42f 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:232460 Archived-At: Hello Alan, Alan Mackenzie writes: > First of all, I will say that I take exception, great exception, to > your means of expressing yourself, both in your post and (even more > so) in the commit you made. It verges on sarcasm, and is totally over > the top and uncalled for. The only sarcastic expression I can read in my post is "volkswagenesque atrocity". All the rest is dead serious. And even the VW reference is appropriate. It's not over the top, and I hope to explain why further down. >> > > with it. > >> > That would leave lots of failed tests in make check. People have >> > already remarked on those failures, implicitly asking me to fix them. > >> Yes. But the correct fix was to revert the commit that broke >> the tests, not this volkswagenesque atrocity. > > The tests are now broken. They make assumptions about CC Mode's workings > which don't hold. Why can't we cooperate to fix them? Of course we can! But let's first analyse what these assumptions are instead of breaking the tests. Perhaps you have the impression that tests are somehow non-real, because they are just code. But you broke actual things in my (and probably other's people usage of C-mode, C++-mode, Awk-mode etc). Things that have been working since Emacs 24.4 and that suddently stop working. If I don't stand up now it'll be like the last time and the bug will stay. And this personally affects my work and other's. You must take that into account. >> > My understanding, from a previous encounter, was that having no failing >> > unit tests was of a high priority. I've only commented a little bit >> > out, I haven't made any permanent, unreverseable changes. >> Then you'll certainly be OK if I revert your two commits myself: > I am not at all OK about this. Why? I'm using the exact same argument you made: It's not permanent or "unreversable", since we have a VCS. >> the commit that broke the tests and the commit that disables the test. >> That is certainly not permanent or unreversable, too! > No, but the amount of work it now requires is greater that immediately > after my commit. ? Have read commit be505726b68d407a44fdcd9c7ac1ef722398532d? You do realize that your commit is still in place right? As we speak users that build Emacs masters or install "emacs-snapshot" builds from Ubuntu are seeing the behaviour you intend. (And they have had electric-pair-mode's features blown away from them). I'm not saying we're going to see pouring bug reports, but the damage is real? >> > With that test in, I got the error message: "No test named >> > `electric-pair-whitespace-chomping-2-at-point-4-in-c++-mode-in-strings= '", >> > and no other tests were performed, leaving an electric-tests.log file = 86 >> > bytes long. That's why I commented it out. This may be some glitch in >> > the testing system. >> I see, you disabled all c++ tests, *sigh* > Yes. I couldn't understand the (undocumented) structure of these tests > well enough to make better targetted amendments. Then you should have asked before. Now, I will explain something. c++ exists in this line (modes '(quote (ruby-mode c++-mode))) But it could very well be (modes '(quote (ruby-mode c-mode awk-mode js-mode c++-mode perl-m= ode))) This runs 1250 tests instead of the usual 400 Selector: t Passed: 1247 Failed: 4 (3 unexpected) ;; this is related to the thing you broke ;; 6 months ago, they're all cc-mode-based Skipped: 0 Total: 1251/1251 That is to say, every mode you plug there should behave exactly the same regarding delimiter insertion and deletion. This is electric-pair-mode's contract. c++-mode is there only as a "lab mouse", it stands for all other cc-mode related modes. >> > > C Mode doesn't use electric-layout mode, but a user can surely >> > > decide he wants to use it in c-mode, can he not?? > >> > No. Certainly not at the moment. > >> No? Will you come to his house and tell him personally not to >> M-x electric-layout-mode? What if that's what he _prefers_? > > That user is purely hypothetical. No, he's not! I use electric-layout-mode from time to time! And Beatrix's problem could very well be solved by it. In fact if you put this in your .emacs today, you're very close to getting c-toggle-auto-newline for braces, and still follow your style variables and whatnot. All without c-toggle-auto-newline. (setq electric-layout-rules '(electric-layout-for-c-style-du-jour)) (setq c--disable-fix-of-bug-33794 t) (defun electric-layout-for-c-style-du-jour (inserted) (when (and (derived-mode-p 'c-mode 'c++-mode) (memq inserted '(?{ ?}))) (save-excursion (backward-char) (c-brace-newlines (c-point-syntax))))) > And if there actually were such a user, I would urge him to use CC > Mode's auto-newline mode, which is much better tailored to CC Mode > than a generic function could be. In what ways? Alan, can you make an objective criticism of electric-layout-mode? I'm not saying it can't be improved, but since you like to bash it so much, can we read a reason? I could use that to improve it. > And if he insisted, then maybe we could look at adding > electric-layout-mode support to CC Mode. But it would be a lot of work > for little return, given that auto-newline mode exists, works, and works > well. In that case I insist! I've shown above that it isn't! Perhaps it's not perfect yet, but certainly doesn't look like a lot of work. Please please read the new electric-layout-mode API. Just M-x describe-variable RET eletric-layout-rules. >> > > These tests pass fine currently. > >> > When I ran them, there were lots of failures, because the tests assumed >> > electric-layout-mode was active. > >> The tests should activate electric-layout-mode temporarily if needed and >> the revert to the previous situation. If they aren't doing this, that i= s what >> needs to be fixed, but it works for me and I don't have electric-layout-= mode, >> too. > > electric-layout-mode doesn't, and can't work with CC Mode, in particular > with the c-electric-.... functions, because these are incompatible with a > non-nil post-self-insert-hook. They aren't generally incompatible, it's just this corner that Beatrix reported that kicked the hornets nest. I've been working with c/c++ and electric-pair-mode for 5+ years now, and it works perfectly if you don't turn on c-auto-newline. But if you want to improve the situation for those that do turn on c-auto-newline, why not create a (defun c-self-insert-command (arg) (let ((post-self-insert-hook (if (some-condition-that-joaot-and-I-have-agreed) nil post-self-insert-hook))) (self-insert-command arg))) It's much easier. >> > > Please revert this fix and lets discuss why you need to disable test= s. > >> > It's not a fix, it's a temporary workaround. > >> With all due respect, I've heard that one before many many times. > >> Do you realize that you've broken many many things about >> electric-pair-mode? > > No, I don't. It would have been good if you could have supplied me with > specific recipes to reproduce where it (electric-pair-mode) goes wrong, > thus allowing me to fix them. The tests themselves supply those recipes. M-x ert-describe-test does really good job, if you don't get hung up.=20 > >> For example Beatrix, and me and everyone else will now lose >> the default "autobalancing" functionality of electric-pair-mode, >> which is its best feature! Autobalance used to work identically in >> most modes, but now you broke that. Is it worth breaking so >> many things just for fixing a much smaller case? > >> Just one example of one of the many cases you broke. > > What is "autobalancing" (the term doesn't appear in elec-pair.el) and how > is it broken? Why can't you cite a recipe to reproduce the alleged > breakage? See the docstring of electric-pair-preserve-balance. And read the test's descriptions. >> #[nil "\300\301!\207" >> [electric-pair-mode 1] >> 2] > > Pardon? Do you mean M-: (electric-pair-mode 1)? Of course I do mean that. Come on, this doc is autogenerated and I was in a hurry last time, I can do some form-walking later-on to provide a perfectly pristine instruction without that glitch. >> The buffer's contents should become: > >> | (()]) | > >> , and point should be at 5. > > Indeed, this is what I observe. That's because the tests are setting c--disable-fix-of-bug-33794 to t! IOW I made my own Volkswagenesque atrocity, but at least mine is easier to identify while we analyse the problem. And it gives me (and perhaps other) the opportunity to develop in a master Emacs in C/C++/whatever without the failures. If you roll back to just after your fix (the version Michael Albinus was using), you'll see it break. You can also see it break if you remove the line of c--disable-fix-of-bug-33794. >> You turn this into (())]) so now I have two unbalanced parenthesis types >> in my buffer. > > Are you sure? Wherein then, lies the difference between your compiled > binary above and M-: (electric-pair-mode 1)? Could this be a bug in > e-p-m, or the byte compiler? No, you're just testing with the wrong version. This one test was picked out of the 85 or so that were broken (just for C++, of course this number is repeated in C/Awk/Pike, whatever by ) >> > Anyhow, surely the implementation of Emacs should not be constrained by >> > its tests? Rather the tests should test the chosen implementation. > >> Really Alan, you completely broke electric-pair-mode by trying to >> reimplement it in cc-mode.el where it was working perfectly but for a sm= all >> glitch (which really isn't its fault). > > I think you are exaggerating somewhat. And you'll find I didn't > "reimplement" electric-pair-mode, even though I was talking about this at > one stage. CC Mode calls electric-pair-post-self-insert-function, but in > a controlled fashion at suitable places. > > And I disagree about your "small glitch" characterisation. Adding > post-self-insert-hook to Emacs broke, fundamentally, c-electric-... This > was the breakage that Beatrix Klebe's bug uncovered. My patch fixed > it. It broke the interaction of c-auto-newline with electric-pair-mode mode as far as I know. This is what was described in bug#33794 by Beatrix Klebe. Does it break more things? Does it break more things related to electric-pair-mode? >> If you're going to reimplement it, reimplement *all* of it (of course yo= u might >> come to the conclusion that its best to use the existing implementation.= ..) > I have used the existing implementation. The function you called is not meant to be called from that context. It's not an "external" function. Shall I rename it electric----pair-post-self-insert-function :-) >> > > If we come to the conclusion that some tests are asserting unreasona= ble >> > > expectations about the functionality you develop, we can disable the= m on a >> > > case by case basis! > >> > I would have done that, indeed tried to do that, but the lack of >> > documentation of electric-pair-test-for, electric-pair-define-test-form >> > and so on, many of them with 8, 9 or more parameters, made that too >> > difficult, given the urgency I felt to produce a workaround. > >> If this is indeed urgent, you can add some conditional check to the >> offending code using 'ert-running-test'. > > Maybe you could do this. I'm not familiar enough with the code in > electric-test.el, important pieces of which, I repeat myself, are wholly > undocumented. It's much easier to do this with a variable that I used: then ert, and some users can set that variable. And it is even easier if we introduce c-self-insert-command and think about the problem. >> > > If on the other hand, if you need to do some work "temporarily", then >> > > the best way to do it without disturbing other people's developments >> > > is to do it in a separate branch. > >> > I fixed bug #33794[*] on master, as I always do with CC Mode bugs (and= most >> > other sorts of bugs, too). That fix is, in principle, sound. I didn't >> > discover the problems with the unit tests till later (though I admit I >> > should have done). > >> It's *not* sound if it breaks tests. At least not without first arguing >> that the tests are placing unreasonable expectations on your code. > > The tests do place such unfounded expectations on the code. In > particular, the contents of post-self-insert-hook. The tests assume modes won't (let ((post-self-insert-hook)) (self-insert-command)) yes. I think that's a reasonable assumption. Besides tests I also assume that! I want my post-self-insert-hook to work. I want to write modes based on it, and I want to order pizzas for Donald Trump every for every character I type! >> > [*] That is, Beatrix Klebe's bug about CC Mode's auto-newlines not >> > working with electric-pair-mode. >> Beatrix Klebe, can, as a workaround, use electric-layout-mode >> perfectly well for her use case (even though you insist she can't) >> Until you can sort out c++-mode. > I've sorted out CC Mode. Beatrix Klebe does not need any workarounds, > given that the bug she reported has been properly fixed. It is likely > she is currently using that fix. You opened 83 or so other bugs times just for C++-mode! Why couldn't you tell Beatrix: look don't use electric-pair-mode in cc-mode when using c-auto-newline??=20=20 >> It's much easier than creating this mess that really interferes >> with other's peoples work. > > I would ask you to consider that the mess was created by you, last night, > and it interferes greatly with my work. This mess was an order of > magnitude greater than the mess you assert I made in > electric-tests.el. How so? Really, explain how! I did *not* remove your fix. I just made it possible to disable it! > I don't consider the temporary changes I made in electric-tests.el were > unreasonable, and if you disagree you could have discussed it with me in > a less disagreeable fashion than what transpired. > > And now, it seems your idea is to leave special purpose code in > CC Mode, just to obviate the need to amend electric-tests.el. As you can > imagine, I'm not keen on that. It is you who left a buggy, incomplete re-implementation of electric-pair-mdoe in cc-mode.el in the first place. It's not needed, Alan. 1. CC-mode can be thinner after all this 2. Beatrix can be happy 3. You could be happy 4. I could be happy 5. Donald Trump would have to pay and eat a million pizzas > That variable must go. It can. Let's use a c-self-insert-command function. Or macro, if you prefer. Then let's check clearly the conditions it should delegate to the previous or the new behaviour. But let's do this is a branch yes? I've just pushed git push origin scratch/resolve-cc-mode-and-e-p-m Total 0 (delta 0), reused 0 (delta 0) remote: Sending notification emails to: emacs-diffs@gnu.org To git.sv.gnu.org:/srv/git/emacs.git * [new branch] scratch/resolve-cc-mode-and-e-p-m -> scratch/= resolve-cc-mode-and-e-p-m > I have another suggestion: you could amend electric-tests.el not to > attempt to use CC Mode at all - or stick with plainer-c-mode, possibly > writing a plainer-c++-mode. After all, the purpose is to test the > electric-... functions, not CC mode. That could save a lot of argument. I would have to write plainer--for that. And give that to people working with me. And teach them to enable these new version. It's ridiculous. But I guess it's an option if we make them default in auto-mode-alist and such. Quite a nuclear option, tho. Jo=E3o