From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: npostavs@users.sourceforge.net Newsgroups: gmane.emacs.bugs Subject: bug#24923: 25.1; Lisp watchpoints Date: Fri, 11 Nov 2016 23:34:33 -0500 Message-ID: <87pom1mi3q.fsf@users.sourceforge.net> References: <87vavun235.fsf@users.sourceforge.net> <83eg2ie3lp.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1478925284 2588 195.159.176.226 (12 Nov 2016 04:34:44 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Sat, 12 Nov 2016 04:34:44 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1 (gnu/linux) Cc: 24923@debbugs.gnu.org To: Eli Zaretskii Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Sat Nov 12 05:34:35 2016 Return-path: Envelope-to: geb-bug-gnu-emacs@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 1c5Q14-0004zG-TY for geb-bug-gnu-emacs@m.gmane.org; Sat, 12 Nov 2016 05:34:11 +0100 Original-Received: from localhost ([::1]:56706 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5Q17-0004cc-LP for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 Nov 2016 23:34:13 -0500 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:44907) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1c5Q11-0004bj-7w for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 23:34:08 -0500 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1c5Q0w-0008PX-A4 for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 23:34:07 -0500 Original-Received: from debbugs.gnu.org ([208.118.235.43]:38999) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1c5Q0w-0008P2-5k for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 23:34:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1c5Q0v-0004NH-RK for bug-gnu-emacs@gnu.org; Fri, 11 Nov 2016 23:34:01 -0500 X-Loop: help-debbugs@gnu.org Resent-From: npostavs@users.sourceforge.net Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Sat, 12 Nov 2016 04:34:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 24923 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 24923-submit@debbugs.gnu.org id=B24923.147892523416801 (code B ref 24923); Sat, 12 Nov 2016 04:34:01 +0000 Original-Received: (at 24923) by debbugs.gnu.org; 12 Nov 2016 04:33:54 +0000 Original-Received: from localhost ([127.0.0.1]:54398 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c5Q0n-0004Mv-Lh for submit@debbugs.gnu.org; Fri, 11 Nov 2016 23:33:53 -0500 Original-Received: from mail-it0-f45.google.com ([209.85.214.45]:38485) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1c5Q0m-0004Mj-El for 24923@debbugs.gnu.org; Fri, 11 Nov 2016 23:33:52 -0500 Original-Received: by mail-it0-f45.google.com with SMTP id q124so16018209itd.1 for <24923@debbugs.gnu.org>; Fri, 11 Nov 2016 20:33:52 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=sender:from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=CL/PF/xbMk/wOxpP9YEaU/VGmMqJYii/+ghLt3WZWcs=; b=vIPI+3mO6KUEwi6k6u7ynGudZrmfG2CXL8cPuHLsxWJxrXhp4QsHrufsdkDqJZXFTz 3Q/YFE1GYy3X/szsiiLl/XY/EJGHligB4FwAnNGj1aVU7YlGqA4c1mfok1T68hzl7fhx u2pzIGtlkVZ6U97avMPC5pgaoCunCHaUrqE5NLhIWRzUVjXMGWEjk+pp5KCr61AXQqV2 Mecd3HqDa5DzD1M+K6DSihUsdM+l7AMx63YfQk4zyZa75OOgdqy0xmPGCV7x3BCTmRFR Bozn6k5b9tO1Naw1742eRtrAZOkZN706vMB5HVbylSYpYpY1EUfX8CDc3NIogDvuEuj5 CQog== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:sender:from:to:cc:subject:references:date :in-reply-to:message-id:user-agent:mime-version; bh=CL/PF/xbMk/wOxpP9YEaU/VGmMqJYii/+ghLt3WZWcs=; b=D53GDWv73mR/M/uI5ZALc50BFDIJc2lqrKkNTrn+KyziNuQXdMNOKgO2WzXViivTVU U77N9nSRVTZ8Z5TjbdxbNiTslcVo5YTKmbXOw2QBhYcQovVVBJyhaPYW4tu033cu0fJ9 0QftGWkPAg+MoQkAEqZAYUy30fDxPi4d535/7rCkOmCgskbYUsNB35Jr5Ri6Cavswkmm S0shKmJbBbhVf5PytH4jv/n29mLmFCortqf9/JxvkizVb9IMlMnmYpkLnE3Pvy8oHgtU hxoOtgmKxqVrEayRbGD8CB2Q6FF78H1QJ5/wmErBbqpxrWb4Jw/IeTvndDQ5/76PGtj/ fAHw== X-Gm-Message-State: ABUngvf75Agl8v3AMvZWM49iK9YciVvT58nkWRZoZSgfB5V4FwG31B7x/ZyvEKjLKXDoqA== X-Received: by 10.107.164.2 with SMTP id n2mr11957238ioe.202.1478925226827; Fri, 11 Nov 2016 20:33:46 -0800 (PST) Original-Received: from zony ([45.2.7.130]) by smtp.googlemail.com with ESMTPSA id o71sm5152752ita.6.2016.11.11.20.33.45 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 11 Nov 2016 20:33:46 -0800 (PST) In-Reply-To: <83eg2ie3lp.fsf@gnu.org> (Eli Zaretskii's message of "Fri, 11 Nov 2016 12:02:42 +0200") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 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.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:125638 Archived-At: Eli Zaretskii writes: > One general comment is that these patches are harder to review due to > whitespace changes TAB->spaces. How about using some non-default > options to "git diff" when generating the patch? Will do for the next one. > >> >From 0507854b885681c2e57bb1c6cb97f898417bd99c Mon Sep 17 00:00:00 2001 > > This changeset was attached twice, for some reason. Oops, I was trying to insert all the attachments at once in gnus via some text manipulation and I forgot to delete the first one I copied from. > >> --- a/src/data.c >> +++ b/src/data.c >> @@ -649,9 +649,10 @@ DEFUN ("makunbound", Fmakunbound, Smakunbound, 1, 1, 0, >> (register Lisp_Object symbol) >> { >> CHECK_SYMBOL (symbol); >> - if (SYMBOL_CONSTANT_P (symbol)) >> + if (XSYMBOL (symbol)->trapped_write == SYMBOL_NOWRITE) >> xsignal1 (Qsetting_constant, symbol); >> - Fset (symbol, Qunbound); >> + else >> + Fset (symbol, Qunbound); >> return symbol; > > Why was this needed? Doesn't xsignal1 never return anymore? Hmm, I'm not sure why I did this. I think this whole hunk can be dropped (as long as SYMBOL_CONSTANT_P is preserved as suggested below). > >> + (Lisp_Object symbol, Lisp_Object watch_function) >> +{ >> + symbol = Findirect_variable (symbol); >> + Lisp_Object watchers = Fget (symbol, Qwatchers); >> + watchers = Fdelete (watch_function, watchers); >> + if (NILP (watchers)) >> + { >> + set_symbol_trapped_write (symbol, SYMBOL_UNTRAPPED_WRITE); >> + map_obarray (Vobarray, harmonize_variable_watchers, symbol); >> + } >> + Fput (symbol, Qwatchers, watchers); >> + return Qnil; > > Would it be more useful for this and add-variable-watcher to return > the list of watchers instead? I don't think it would be especially useful, but it's easy to do so. > >> +/* Value is non-zero if symbol cannot be changed through a simple set, >> + i.e. it's a constant (e.g. nil, t, :keywords), or it has some >> + watching functions. */ >> >> INLINE int >> -(SYMBOL_CONSTANT_P) (Lisp_Object sym) >> +(SYMBOL_TRAPPED_WRITE_P) (Lisp_Object sym) >> { >> - return lisp_h_SYMBOL_CONSTANT_P (sym); >> + return lisp_h_SYMBOL_TRAPPED_WRITE_P (sym); >> } >> >> /* Placeholder for make-docfile to process. The actual symbol >> @@ -3289,6 +3299,12 @@ set_symbol_next (Lisp_Object sym, struct Lisp_Symbol *next) >> XSYMBOL (sym)->next = next; >> } >> >> +INLINE void >> +make_symbol_constant (Lisp_Object sym) >> +{ >> + XSYMBOL (sym)->trapped_write = SYMBOL_NOWRITE; >> +} >> + > > So we will have make_symbol_constant, but no SYMBOL_CONSTANT_P? Isn't > that confusing? Some C code may wish to know whether a symbol is > constant, not just either constant or trap-on-write; why not give them > what they want? I suppose I took Stefan's advice to replace SYMBOL_CONSTANT_P with SYMBOL_TRAPPED_WRITE_P too literally. Although it's almost always the case that C code checking if a symbol is constant wants to write to the symbol, in which case it should be doing a 3-way switch (writable, trapped write, or constant). But there is one exception in Fmakunbound (mentioned above), so I guess we may as well keep it. > >> + (_ (format "watchpoint triggered %S" (cdr args)))) > > Can you give a couple of examples of this, with %S shown explicitly? > I'm not sure whether the result will be self-explanatory. You mean examples of this this clause being used? It was meant more as a catchall in case some watch types were missed by the previous clauses. It shouldn't really ever happen unless the debugger and watchpoint code get out of sync. Do you think it would be better to just signal an error? (although would signalling an error while the debugger is invoked cause trouble?) > >> +;;;###autoload >> +(defun debug-watch (variable) >> + (interactive >> + (let* ((var-at-point (variable-at-point)) >> + (var (and (symbolp var-at-point) var-at-point)) >> + (val (completing-read >> + (concat "Debug when setting variable" >> + (if var (format " (default %s): " var) ": ")) > > I think all the other commands/variables similar to this one are named > debug-on-SOMETHING, so perhaps this one should also have such a name. > Like debug-on-setting-variable, perhaps? Hah, I initially called this debug-on-set (https://lists.nongnu.org/archive/html/emacs-devel/2015-11/txtyjJDztIULG.txt), and then you suggested debug-watch instead (https://lists.nongnu.org/archive/html/emacs-devel/2015-11/msg02017.html). An alias probably makes sense, how about debug-on-variable-change? (since it catches some changes other than `set'.)