unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
From: Stefan Monnier <monnier@iro.umontreal.ca>
To: Kelly Dean <kelly@prtime.org>
Cc: emacs-devel@gnu.org
Subject: Re: [PATCH] (Updated) Run hook when variable is set
Date: Sat, 21 Feb 2015 15:51:33 -0500	[thread overview]
Message-ID: <jwv4mqfj9gy.fsf-monnier+emacs@gnu.org> (raw)
In-Reply-To: <HGBvdMciIbdx8At48cuBzdhnw7ZhtFedJlH41DHA4PG@local> (Kelly Dean's message of "Sat, 21 Feb 2015 14:18:02 +0000")

>>> +#define SYM_CONST 1
>>> +#define SYM_HOOKED 2
>> constant and hooked at the same time is nonsensical, so these aren't two
>> independent bits, instead `vetted' is a 3-valued field.  An `enum'
>> sounds right.
> The compiler will encode the enum into Lisp_Symbol's bitfield, so that will
> produce no change in the compiled code.

I know.  My objection is mostly against the comment(s).
They're basically a left over from when you had 2 separate fields but
are out-of-date w.r.t the current code.

> And SYM_CONST and SYM_HOOKED are better than sprinkling the code with
> 1 and 2 as magic numbers, for the same reason that «false» and «true»
> are better than 0 and 1 for booleans.

I definitely don't want magic constants, which is why I said "An `enum'
sounds right".

> Note that those constant definitions have been there ever since I originally
> combined the constant and hooked fields in the patch I submitted on Feb 9th.

I know.

>>> -static void grow_specpdl (void);
>>> +static inline void grow_specpdl (void);
>> What happens if we don't inline grow_specpdl?
> That's just an optimization since it's in the critical path of specbind, as
> I noted in my message when I made this change on Feb 9th.

But that's orthogonal to the "variable hook" functionality, right?
So I think we can keep this for some later changes.

> I didn't have to. It's just a minor optimization to avoid an extra
> conditional branch before the common case (SYMBOL_PLAINVAL) on the critical

Ah, you're moving the expected common case to the first position.
That sounds OK.  But maybe we can keep this for some later changes, tho.

> Neither would I, if void could never shadow non-void.

It definitely can.  It almost never does, tho.

> I don't see why mark it special, but t and nil are marked special, so
> I decided to do this for the sake of consistency, in case there was some
> reason constants should be marked special.

I'm not sure exactly why t and nil are marked special, but the effect it
has is to prevent their use a lexical vars, and hence prevent their use
as let-bound vars altogether.

The "constant" bit prevents a dynamically scoped (let ((t <foo>)) ...)
while the "special" bit prevents a lexically scoped (let ((t <foo>)) ...).

The first would be dangerous, while the second would be perfectly safe,
but it might be a bit confusing for the programmer who expects t and nil
to always refer to the special predefined constants.

I can't imagine how a chunk of code you try to let-bind the symbol
that's the void-value, marking it "special" is pretty paranoid.

> Because if you could, then as I explained in my previous message, you could
> convert a setq into a makunbound,

I don't think that's a serious problem.

> and since I changed setq to return the override value,

By now, I think I've convinced myself that this an error.

> It would be annoying in the case of (if (setq x y) ...), if you override the
> setting of x but the «if» doesn't respect your choice.

It all depends on what you mean by "override the setting".  In the above
code, the usual intention is "compute y, test the result, and remember
it in x for later reuse".  If the setter function decides to put some
other value into `x', that doesn't mean that the test should also return
another value.

Here's another case:

   (setq x (setq y <foo>))
vs
   (setq y (setq x <foo>))

if you hook `x' and change the value to which it's set, do you really
think the programmer expect the above two expressions to
behave differently?

>> Try M-: (list (setq auto-hscroll-mode 2) auto-hscroll-mode) RET
> Whether or not hooked, you get (2 t).

That's not the point: the `setq' returned the value passed to it, and
not the value to which the variable was set.  This is not done with your
variable hooks, but by some pre-existing code instead, but the principle
is the same.  And in that pre-existing similar situation we made the
opposite choice to the one you're making now.

>>> -  if (sym->constant)
>>> +  if (SYM_CONST_P (sym))
>>> error ("Symbol %s may not be frame-local", SDATA (SYMBOL_NAME (variable)));
>> I think we can keep sym->constant here.
> That would be a bug, since it would signal an error if the symbol is
> hooked. Hooking a symbol must not change the behavior of Emacs.

No, as I said, I'm happy to introduce such "bugs" for frame-local vars.

>> we want to inflict pain on the few rare remaining users of
>> frame-local.
> Just deleting all the code for frame-local would accomplish that
> more effectively.

I've done that in my local branch, and every new release reduces the
support of frame-local vars to some extent.  I haven't thought enough
about when we should finally remove support for them altogether (maybe
25.1 is a bit early still), but in the mean time, there's no reason to
support interaction between them and new features.


        Stefan



  reply	other threads:[~2015-02-21 20:51 UTC|newest]

Thread overview: 110+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-04-20  2:54 Proposal to change cursor appearance to indicate region activation Kelly Dean
2013-04-20  7:23 ` Drew Adams
2015-01-22  5:38   ` [PATCH] " Kelly Dean
2015-01-22 14:25     ` Stefan Monnier
2015-01-23  3:08       ` [PATCH] " Kelly Dean
2015-01-23  4:55         ` Stefan Monnier
2015-01-23 11:07           ` Kelly Dean
2015-01-23 17:49             ` Drew Adams
2015-01-24  3:06               ` Kelly Dean
2015-01-24  4:52                 ` Stefan Monnier
2015-01-24  9:22                   ` Kelly Dean
2015-01-25 14:29                     ` Stefan Monnier
2015-01-28  9:15                       ` [PATCH] Run hook when variable is set Kelly Dean
2015-01-28  9:23                         ` [PATCH] Proposal to change cursor appearance to indicate region activation Kelly Dean
2015-01-28 11:24                           ` David Kastrup
2015-01-28 12:13                             ` David Kastrup
2015-01-29 10:46                             ` Kelly Dean
2015-01-29 11:16                               ` David Kastrup
2015-01-30  7:20                                 ` Kelly Dean
2015-01-30  9:19                                   ` David Kastrup
2015-01-30 10:05                                     ` Kelly Dean
2015-01-30 10:12                                       ` David Kastrup
2015-01-30  9:43                                   ` Kelly Dean
2015-01-28 19:25                         ` [PATCH] Run hook when variable is set Stefan Monnier
2015-01-29  8:20                           ` Kelly Dean
2015-01-29  8:28                             ` Lars Ingebrigtsen
2015-01-29 14:58                             ` Stefan Monnier
2015-01-30  7:34                               ` Kelly Dean
2015-01-30 15:55                                 ` Stefan Monnier
2015-01-31  9:18                                   ` Kelly Dean
2015-01-31 20:48                                     ` Stefan Monnier
2015-02-02  5:40                                       ` Kelly Dean
2015-02-02 15:57                                         ` Stefan Monnier
2015-02-03 19:56                                           ` Kelly Dean
2015-02-03 22:49                                             ` Stefan Monnier
2015-02-05  3:10                                               ` [PATCH] (Updated) " Kelly Dean
2015-02-05 13:57                                                 ` Stefan Monnier
2015-02-06  5:34                                                   ` Kelly Dean
2015-02-06 14:42                                                     ` Stefan Monnier
2015-02-07 12:27                                                       ` Kelly Dean
2015-02-07 15:09                                                         ` Stefan Monnier
2015-02-09  3:24                                                           ` Kelly Dean
2015-02-12 19:58                                                             ` Stefan Monnier
2015-02-13 23:08                                                               ` Kelly Dean
2015-02-14  0:55                                                                 ` Stefan Monnier
2015-02-14 22:19                                                                   ` Kelly Dean
2015-02-15 20:25                                                                     ` Stefan Monnier
2015-02-17  2:22                                                                       ` Kelly Dean
2015-02-17 23:07                                                                         ` Richard Stallman
2015-02-18  3:19                                                                           ` The purpose of makunbound (Was: Run hook when variable is set) Kelly Dean
2015-02-18  5:48                                                                             ` The purpose of makunbound Stefan Monnier
2015-02-18  8:51                                                                               ` Kelly Dean
2015-02-18 14:34                                                                                 ` Stefan Monnier
2015-02-18 18:53                                                                                   ` Kelly Dean
2015-02-18 22:42                                                                                     ` Stefan Monnier
2015-02-19 10:36                                                                                       ` Kelly Dean
2015-02-22  0:18                                                                                   ` Kelly Dean
2015-02-19 10:45                                                                           ` Kelly Dean
2015-02-19 13:33                                                                             ` Stefan Monnier
2015-02-19 23:51                                                                               ` Kelly Dean
2015-02-20  1:59                                                                                 ` Stefan Monnier
2015-02-20  9:35                                                                                   ` Kelly Dean
2015-02-20 16:55                                                                                     ` Stefan Monnier
2015-02-20  2:58                                                                                 ` Stephen J. Turnbull
2015-02-20  0:56                                                                             ` Richard Stallman
2015-02-20  9:02                                                                               ` Kelly Dean
2015-02-20 15:41                                                                                 ` Richard Stallman
2015-02-21  5:45                                                                                   ` Stephen J. Turnbull
2015-02-22  0:32                                                                                     ` Kelly Dean
2015-02-22  8:45                                                                                       ` Andreas Schwab
2015-02-18  5:15                                                                         ` [PATCH] (Updated) Run hook when variable is set Kelly Dean
2015-02-18 22:37                                                                           ` Stefan Monnier
2015-02-18 22:37                                                                         ` Stefan Monnier
2015-02-19 10:35                                                                           ` Kelly Dean
2015-02-19 13:30                                                                             ` Stefan Monnier
2015-02-20  6:48                                                                               ` Kelly Dean
2015-02-20 19:29                                                                                 ` Stefan Monnier
2015-02-21 14:18                                                                                   ` Kelly Dean
2015-02-21 20:51                                                                                     ` Stefan Monnier [this message]
2015-02-22  0:32                                                                                       ` Kelly Dean
2015-02-22 10:40                                                                                         ` Stephen J. Turnbull
2015-02-22 21:35                                                                                         ` Stefan Monnier
2015-02-23  3:09                                                                                           ` Kelly Dean
2015-02-23  4:19                                                                                             ` Stefan Monnier
2015-02-20 20:27                                                                               ` Proposal for debugging/testing option Kelly Dean
2015-02-24 16:28                                                                                 ` Stefan Monnier
2015-02-14 20:37                                                               ` [PATCH] (Updated) Run hook when variable is set Johan Bockgård
2015-02-15 19:36                                                                 ` Stefan Monnier
2015-02-15 19:53                                                                   ` Patches: inline vs. attachment, compressed vs. uncompressed. [was: Run hook when variable is set] Alan Mackenzie
2015-02-06  9:55                                                   ` [PATCH] (Updated) Run hook when variable is set Kelly Dean
2015-01-30 23:29                                 ` [PATCH] " Richard Stallman
2015-01-31  9:23                                   ` Kelly Dean
2015-01-31 23:16                                     ` Richard Stallman
2015-02-02  5:41                                       ` Kelly Dean
2015-02-01  2:04                               ` Alexis
2015-02-01  4:05                                 ` Stefan Monnier
2015-02-01  8:58                                   ` David Kastrup
2015-01-29 16:06                             ` Eli Zaretskii
2015-01-30  7:14                               ` Kelly Dean
2015-01-30  9:08                                 ` Eli Zaretskii
2015-01-23 20:34             ` [PATCH] Proposal to change cursor appearance to indicate region activation Stefan Monnier
2015-01-24  0:25               ` Kelly Dean
2015-01-23 10:01         ` Tassilo Horn
2015-01-23 17:49           ` Drew Adams
2015-01-23 10:06         ` Eli Zaretskii
2015-01-23 11:40           ` Kelly Dean
2015-01-23 11:56             ` Eli Zaretskii
2015-01-22  5:41   ` Kelly Dean
2013-11-23 13:34 ` Stefan Monnier
2013-11-23 20:25   ` Drew Adams

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/emacs/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=jwv4mqfj9gy.fsf-monnier+emacs@gnu.org \
    --to=monnier@iro.umontreal.ca \
    --cc=emacs-devel@gnu.org \
    --cc=kelly@prtime.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://git.savannah.gnu.org/cgit/emacs.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).