[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?
Stefan