[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/2] Add frame-pointer-visible-p

From: Stefan Monnier
Subject: Re: [PATCH 1/2] Add frame-pointer-visible-p
Date: Sun, 17 Oct 2010 15:11:35 -0400
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/24.0.50 (gnu/linux)

Looks pretty good, thank you, except for:

> +  if (frame && FRAMP (frame) && !XFRAME(frame)->pointer_invisible)

- Testing "frame" is wrong: it's a Lisp_Object, not a boolean (yes,
  I know Lisp_Object is often defined as `int' or `long', so the
  compiler may not complain, but it's still a bug: use
  "./configure --enable-use-lisp-union-type" to let the compiler help
  you find such bugs).

- FRAMP is an obvious typo.

- There should be a space between XFRAME and (
- You should add a CHECK_FRAME (frame) so as to signal an error rather
  than return nil if the arg is not a frame.

- I prefer

  return (XFRAME(frame)->pointer_invisible ? Qnil : Qt);

> +EXFUN (Fframe_pointer_visible_p, 1);

As long as the function is not called from C code, there's no need to
EXFUN it (not that it hurts, tho).

One more thing: while I see that the C code currently stores the
visibility in the frame data-structure, I'm not completely sure if this
data is truly frame-specific as opposed to terminal-specific.
Can someone confirm this issue?


reply via email to

[Prev in Thread] Current Thread [Next in Thread]