8:52 a.m.
APPROVE COMMIT 21.5
# HG changeset patch
# Parent a3aa49cd5d7559d6947d815a6aa78413e0010c30
Quiet clang about vdb_fault_handler prototype.
* * *
Quiet compiler warnings.
* * *
Combine with quiet-compiler, please.
diff -r a3aa49cd5d75 lwlib/ChangeLog
--- a/lwlib/ChangeLog Thu Nov 23 18:48:06 2017 +0000
+++ b/lwlib/ChangeLog Sat Nov 25 17:48:54 2017 +0900
＠＠ -1,3 +1,7 ＠＠
+2017-11-23 Stephen J. Turnbull <stephen(a)xemacs.org>
+
+ * lwlib-colors.c (xft_convert_color): Remove unused variable.
+
2017-11-12 Aidan Kehoe <kehoea(a)parhasard.net>
* lwlib-internal.h:
diff -r a3aa49cd5d75 lwlib/lwlib-colors.c
--- a/lwlib/lwlib-colors.c Thu Nov 23 18:48:06 2017 +0000
+++ b/lwlib/lwlib-colors.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -355,7 +355,6 ＠＠
xft_convert_color (Display *dpy, Colormap cmap, Visual *visual, int c, int dim)
{
static XColor color; /* #### why is this static ?? */
- XftColor result;
color.pixel = c;
XQueryColor(dpy, cmap, &color);
diff -r a3aa49cd5d75 src/ChangeLog
--- a/src/ChangeLog Thu Nov 23 18:48:06 2017 +0000
+++ b/src/ChangeLog Sat Nov 25 17:48:54 2017 +0900
＠＠ -1,3 +1,19 ＠＠
+2017-11-24 Stephen J. Turnbull <stephen(a)xemacs.org>
+
+2017-11-23 Stephen J. Turnbull <stephen(a)xemacs.org>
+
+ * sysdep.c (total_data_usage):
+ Document that sbrk(0) usage is still OK.
+
+ * window.c (window_divider_width):
+ * window-impl.h (MODELINE_OFF_SHADOW_THICKNESS_ADJUSTED):
+ Use EMACS_INT_ABS to shut up compiler.
+
+ * lread.c (read1): Comment too-clever logic.
+
+ * vdb-posix.c (vdb_fault_handler):
+ Add prototype to quiet clang (at least 4.0 and 5.0).
+
2017-11-23 Aidan Kehoe <kehoea(a)parhasard.net>
* config.h.in:
diff -r a3aa49cd5d75 src/lread.c
--- a/src/lread.c Thu Nov 23 18:48:06 2017 +0000
+++ b/src/lread.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -2644,6 +2644,9 ＠＠
while ((c = readchar (readcharfun)) >= 0
&& c >= '0' && c <= '9')
{
+ /* #### I think this logic is wrong. Skip can be in the
+ range 0x40000000-0xffffffff, the overflow will not
+ trigger, but this won't fit in a fixnum. */
skip = (10 * skip) + (c - '0');
if (oskip <= skip)
{
diff -r a3aa49cd5d75 src/sysdep.c
--- a/src/sysdep.c Thu Nov 23 18:48:06 2017 +0000
+++ b/src/sysdep.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -1991,6 +1991,9 ＠＠
#endif
#ifndef WIN32_ANY
+ /* clang emits a deprecation warning but this usage documented OK.
+ On Linux, apparently can use /proc filesystem; on Mac OS X need to use
+ mach_vm_region_info which looks annoying. */
void *data_end = sbrk (0);
#else
void *data_end = maximum_address_seen;
diff -r a3aa49cd5d75 src/vdb-posix.c
--- a/src/vdb-posix.c Thu Nov 23 18:48:06 2017 +0000
+++ b/src/vdb-posix.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -55,6 +55,7 ＠＠
sighandler_t segv_oact, bus_oact;
#endif /* USE_SIGNAL */
+void vdb_fault_handler (FAULT_HANDLER_ARGUMENTS);
void vdb_remove_signal_handler (void);
void
diff -r a3aa49cd5d75 src/window-impl.h
--- a/src/window-impl.h Thu Nov 23 18:48:06 2017 +0000
+++ b/src/window-impl.h Sat Nov 25 17:48:54 2017 +0900
＠＠ -269,12 +269,12 ＠＠
#define WINDOW_HAS_MODELINE_P(w) (!NILP (w->has_modeline_p))
#define MODELINE_OFF_SHADOW_THICKNESS_ADJUSTED(win) \
- abs ((!WINDOW_HAS_MODELINE_P (win) \
- ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
- ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
+ EMACS_INT_ABS ((!WINDOW_HAS_MODELINE_P (win) \
+ ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
+ ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
: ((XFIXNUM (win->modeline_shadow_thickness) < -1) \
? XFIXNUM (win->modeline_shadow_thickness) + 1 \
- : XFIXNUM (win->modeline_shadow_thickness))) \
+ : XFIXNUM (win->modeline_shadow_thickness))) \
: XFIXNUM (win->modeline_shadow_thickness)))
#define MODELINE_SHADOW_THICKNESS(win) \
diff -r a3aa49cd5d75 src/window.c
--- a/src/window.c Thu Nov 23 18:48:06 2017 +0000
+++ b/src/window.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -1062,7 +1062,7 ＠＠
return
XFIXNUM (w->vertical_divider_line_width)
+ 2 * XFIXNUM (w->vertical_divider_spacing)
- + 2 * abs (XFIXNUM (w->vertical_divider_shadow_thickness));
+ + 2 * EMACS_INT_ABS (XFIXNUM (w->vertical_divider_shadow_thickness));
else
return XFIXNUM (w->vertical_divider_line_width) == 0 ? 0 : 1;
}
10:57 a.m.
Thanks, Steve!
Ar an cúigiú lá is fiche de mí na Samhain, scríobh Stephen J. Turnbull:
APPROVE COMMIT 21.5
* config.h.in:
diff -r a3aa49cd5d75 src/lread.c
--- a/src/lread.c Thu Nov 23 18:48:06 2017 +0000
+++ b/src/lread.c Sat Nov 25 17:48:54 2017 +0900
＠＠ -2644,6 +2644,9 ＠＠
while ((c = readchar (readcharfun)) >= 0
&& c >= '0' && c <= '9')
{
+ /* #### I think this logic is wrong. Skip can be in the
+ range 0x40000000-0xffffffff, the overflow will not
+ trigger, but this won't fit in a fixnum. */
skip = (10 * skip) + (c - '0');
if (oskip <= skip)
This doesn’t have to fit into a fixnum, except for the error message, which
will work ok with make_unsigned_integer and bignum support.
diff -r a3aa49cd5d75 src/window-impl.h
--- a/src/window-impl.h Thu Nov 23 18:48:06 2017 +0000
+++ b/src/window-impl.h Sat Nov 25 17:48:54 2017 +0900
＠＠ -269,12 +269,12 ＠＠
#define WINDOW_HAS_MODELINE_P(w) (!NILP (w->has_modeline_p))
#define MODELINE_OFF_SHADOW_THICKNESS_ADJUSTED(win) \
- abs ((!WINDOW_HAS_MODELINE_P (win) \
- ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
- ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
+ EMACS_INT_ABS ((!WINDOW_HAS_MODELINE_P (win) \
+ ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
+ ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
: ((XFIXNUM (win->modeline_shadow_thickness) < -1) \
? XFIXNUM (win->modeline_shadow_thickness) + 1 \
- : XFIXNUM (win->modeline_shadow_thickness))) \
+ : XFIXNUM (win->modeline_shadow_thickness))) \
: XFIXNUM (win->modeline_shadow_thickness)))
I had been holding off on making that change because all these values are
pixel widths, they really shouldn’t get values up to #x3fffffffffffffff, so
abs() should be fine with appropriate range restrictions. It would be nice
if there were integer specifiers that allowed more limitations on values.
The natnum specifier is the closest thing at the moment, and “greater than
-1” isn’t much of a restriction.
--
‘As I sat looking up at the Guinness ad, I could never figure out /
How your man stayed up on the surfboard after forty pints of stout’
(C. Moore)
6:07 a.m.
Aidan Kehoe writes:
This doesn’t have to fit into a fixnum, except for the error
message, which will work ok with make_unsigned_integer and bignum
support.
(1) I don't think we're guaranteed to have bignum support.
(2) In that range the file is in error (character indicies *do* have
to fit in a fixnum), but no error is reported. This will blow up
in line 2672.
> - abs ((!WINDOW_HAS_MODELINE_P (win) \
> - ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
> - ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
> + EMACS_INT_ABS ((!WINDOW_HAS_MODELINE_P (win) \
> + ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
> + ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
I had been holding off on making that change because all these
values are pixel widths, they really shouldn’t get values up to
#x3fffffffffffffff, so abs() should be fine with appropriate range
restrictions. It would be nice if there were integer specifiers
that allowed more limitations on values. The natnum specifier is
the closest thing at the moment, and “greater than -1” isn’t much
of a restriction.
Yeah, I thought about that, but as you say, we don't have a good way
to say "no pixel size bigger than 8K exists outside of a hospital" on
the one hand, and on the other they're both declared as functions
(presumably inlined). I guess there may be a slightly higher cost for
long than int on 64-bit machines, but I doubt this function is called
enough to make a big difference. Ditto space savings.
I'll revert it if you want, but I really dislike warnings, and think
we probably have better things to do than avoid labs() in one function.
Regards,
Steve
7:38 a.m.
Ar an t-ochtú lá is fiche de mí na Samhain, scríobh Stephen J. Turnbull:
Aidan Kehoe writes:
> This doesn’t have to fit into a fixnum, except for the error
> message, which will work ok with make_unsigned_integer and bignum
> support.
(1) I don't think we're guaranteed to have bignum support.
Right. Without bignum support, we’ll still get an error, just a more
confusing one. Which is well and good, it’s motivation to build with bignum
support.
(2) In that range the file is in error (character indicies *do* have
to fit in a fixnum), but no error is reported. This will blow up
in line 2672.
It doesn’t. Test code here:
https://parhasard.net/xemacs/very-long-comment.el.html
Adjust very-long-comment-length and very-long-comment-readchar-prologue to
taste; the limiting factor is likely your appetite for the heap-allocation
frenzy as each increment and each numeric comparison creates a new bignum
when the index gets above most-positive-fixnum.
> > - abs ((!WINDOW_HAS_MODELINE_P (win) \
> > - ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
> > - ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
> > + EMACS_INT_ABS ((!WINDOW_HAS_MODELINE_P (win) \
> > + ? ((XFIXNUM (win->modeline_shadow_thickness) > 1) \
> > + ? XFIXNUM (win->modeline_shadow_thickness) - 1 \
> I had been holding off on making that change because all these
> values are pixel widths, they really shouldn’t get values up to
> #x3fffffffffffffff, so abs() should be fine with appropriate range
> restrictions. It would be nice if there were integer specifiers
> that allowed more limitations on values. The natnum specifier is
> the closest thing at the moment, and “greater than -1” isn’t much
> of a restriction.
Yeah, I thought about that, but as you say, we don't have a good way
to say "no pixel size bigger than 8K exists outside of a hospital" on
the one hand, and on the other they're both declared as functions
(presumably inlined). I guess there may be a slightly higher cost for
long than int on 64-bit machines, but I doubt this function is called
enough to make a big difference. Ditto space savings.
I'll revert it if you want, but I really dislike warnings, and think
we probably have better things to do than avoid labs() in one function.
We silence all these warnings all the time, and they’re usually false
positives. The whole point of turning them on is the occasional bug they
flush out. And this is an actual bug, and now I grep for them, *all* of the
natnum and integer specifiers in the C code would benefit from a similar
restriction, they’re all pixel widths. It’s not just one place.
Here’s a doozy:
xemacs-21.5-b34 -vanilla &
C-x 2 ;; (split the *scratch* window)
(set-specifier vertical-divider-shadow-thickness `((global (nil .
,most-positive-fixnum)))) C-j
;; Watch XEmacs crash
--
‘As I sat looking up at the Guinness ad, I could never figure out /
How your man stayed up on the surfboard after forty pints of stout’
(C. Moore)
5:35 a.m.
Ar an naoú lá is fiche de mí na Samhain, scríobh Aidan Kehoe:
Here’s a doozy:
xemacs-21.5-b34 -vanilla &
C-x 2 ;; (split the *scratch* window)
(set-specifier vertical-divider-shadow-thickness `((global (nil .
,most-positive-fixnum)))) C-j
;; Watch XEmacs crash
The keyboard input should have been C-x 3, to split the window horizontally,
Oddly for me, today, it doesn’t crash, where yesterday it did. It’s a clear
bug, though.
--
‘As I sat looking up at the Guinness ad, I could never figure out /
How your man stayed up on the surfboard after forty pints of stout’
(C. Moore)
1 a.m.
Aidan Kehoe writes:
The keyboard input should have been C-x 3, to split the window
horizontally, Oddly for me, today, it doesn’t crash, where
yesterday it did. It’s a clear bug, though.
Agreed. However, it's not obvious to me how to fix it. Some of those
dimensions (dividers) are normally going to fit in a byte; I think X
protocol has dimensions and positions as shorts (I'm not even sure
they're unsigned -- I think dims are unsigned, positions signed --
which means 10K is not feasible!) Xorg itself does ZERO checking, and
assumes that the programmer (or OS, in the case of Mac OS X and zombie
file descriptors) will never violate any of its undocumented
implementation-dependent assumptions.
So I suppose the ideal is to do checking when setting these variables,
probably dependent on relevant screen dimensions? What's practical?
("Damned if I know" is of course an appropriate answer at this stage
:-/).
Thanks again for analyzing and testing this stuff.
Steve
2335
days inactive
2341
days old
5 comments
3 participants
participants (3)
-
Aidan Kehoe -
Stephen J. Turnbull -
Stephen J. Turnbull