Subject: New signal stuff (and something else)

From: Chris Rankin <[email-masked]>
Date: Tue, 05 Jan 1999 21:24:01 -0500

These patches are against the latest code from CVS. There is also some
provisional precompiler stuff that will need integrating into the
config. I've tried to accommodate both POSIX.1 and BSD signals depending
on two #defines USE_POSIX and USE_BSD, with USE_POSIX as the default.
The main point is that this #define stuff has to appear somehow before
the line #include <signal.h> in the source file so that it can #define
_BSD_SOURCE.

Chris.

---

From: Tim Phipps <[email-masked]>
Date: Wed, 06 Jan 1999 10:42:00 +0000

Chris Rankin wrote:
>                           Name: fvwm-placement.diff
>    fvwm-placement.diff    Type: unspecified type (application/octet-stream)
>                       Encoding: base64

>From the something else patch:
+  typedef int(*fnPlacement_t)(FvwmWindow*, int, int, int*, int*, int,
int);
+  static const fnPlacement_t placement[2] = { NormalSmartPlacement,
+                                              CleverPlacement };
+  assert(False==0);
+  assert(True==1);
+  assert( (Scr.SmartPlacementIsClever == False) ||
+          (Scr.SmartPlacementIsClever == True) );

What does assert() do?

Cheers,
Tim.
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 6 Jan 1999 11:19:11 +0100

> the line #include <signal.h> in the source file so that it can #define

I've applied the patches but have no knowledge about these
portability issues. Steve or Paul (PDS) will have to take
care of these.

Bye

Dominik

--
Dominik Vogt, [email-masked]
Reply-To: [email-masked]

---

From: Dominik Vogt <[email-masked]>
Date: Wed, 6 Jan 1999 13:24:58 +0100

> +          (Scr.SmartPlacementIsClever == True) );

If the condition is false it kills your program. This is 
somewhat better than continuing to run with a parameter
that should never occur.

Bye

Dominik

---

From: Tim Phipps <[email-masked]>
Date: Wed, 06 Jan 1999 12:52:42 +0000

> somewhat better than continuing to run with a parameter

It looks a bit over-the-top to do these tests every time a window is
placed. The first two are redundant and looking at the code
Scr.SmartPlacementIsClever is only ever set to True or False.  It's an
int and maybe it should be a BOOL which would make the third assert
redundant.  Am I right in thinking that these asserts are making sure
that using Scr.SmartPlacementIsClever as a vector into a choice of two
placement functions is in range? If so why not just use:

if (Scr.SmartPlacementIsClever)
  return cleverfunc(...);
else
  return smartfunc(...);

and not have to worry.

Cheers,
Tim.

---

From: Chris Rankin <[email-masked]>
Date: Wed, 6 Jan 1999 08:59:48 -0500 (EST)

> What does assert() do?

It's debug-code that disappears unless you #define NDEBUG. Basically,
I'm checking that True and False have the integer values I expect so
that I can use the Boolean SmartPlacementIsClever to index correctly
into my array.

Ideally, SmartPlacementIsClever would be an enumerated type like

typedef enum {
               NORMAL_PLACEMENT=0,
               CLEVER_PLACEMENT
             } SMART_PLACEMENT_MODE;

That way, it would be trivial to add further placement modes in the
future and the index values would be assured.

Chris.

---

From: Chris Rankin <[email-masked]>
Date: Wed, 6 Jan 1999 09:07:48 -0500 (EST)

> if (Scr.SmartPlacementIsClever)

It's a development-time check. assert() doesn't produce a single byte
of code if NDEBUG isn't #defined. The advantage of using a vector is
that it avoids branching and is therefore faster. It is also more
extensible in the longer term.

Chris.

---

From: [email-masked] (Paul D. Smith)
Date: 06 Jan 1999 10:04:21 -0500

%% Chris Rankin <[email-masked]> writes:

  cr> It's a development-time check. assert() doesn't produce a single
  cr> byte of code if NDEBUG isn't #defined.

You've got this backwards, Chris; assert() will be compiled out only if
NDEBUG _IS_ defined.

A standard build and install will always have the assert() calls
compiled in and utilized.

-- 
-------------------------------------------------------------------------------
 Paul D. Smith <[email-masked]>         Network Management Development
 "Please remain calm...I may be mad, but I am a professional." --Mad Scientist
-------------------------------------------------------------------------------
   These are my opinions---Nortel Networks takes no responsibility for them.
--
Visit the official FVWM web page at <URL:http://www.hpc.uh.edu/fvwm/>.
To unsubscribe from the list, send "unsubscribe fvwm-workers" in the
body of a message to [email-masked].
To report problems, send mail to [email-masked].

---

From: [email-masked] (Paul D. Smith)
Date: 06 Jan 1999 10:10:46 -0500

%% Dominik Vogt <[email-masked]> writes:

  >> I've tried to accommodate both POSIX.1 and BSD signals depending

  dv> I've applied the patches but have no knowledge about these
  dv> portability issues. Steve or Paul (PDS) will have to take
  dv> care of these.

In general this isn't the way you want to do things with configure.

Instead of generic flags like USE_POSIX and USE_BSD, you test for very
targeted, specific things and use defines for those.  For example, you
would test for sigaction() and put invocations inside a:

  #if HAVE_SIGACTION
    ...
  #endif

Or, if you need a number of things you might test for all of them in
configure and then have some preprocessor magic like:

  #if defined(HAVE_SIGACTION) && defined(HAVE_SIGLONGJMP)
  # define FVWM_USE_POSIXY_SIGNALS
  #endif

The compelling reason for this is many systems implement _some_, but not
all, of some of the standards.  For example, SunOS has memcpy() and
memcmp(), but not memmove().

Also, defining things like _BSD_SOURCE before standard headers is, I
think, not a good idea.  You never really know what that will do on
various systems, it could well screw things up a good bit.  Some
systems, for example, might revert to a 100% BSD implementation and
other POSIX-ish functions the fvwm uses would then be unavailable.

I'll try to find some time to examine the changes you made, but what
would be very useful is if you posted a detailed summary of the two
options, how signal handling is done in each, and what system functions
you're relying on for each one.

---

From: [email-masked] (Paul D. Smith)
Date: 07 Jan 1999 15:51:35 -0500

Hi all;

I really appreciate the work Chris is doing on signals for fvwm, but
nevertheless I feel I must complain a little :).  And I'm not really
targeting Chris because others have done this, too--it's his misfortune
to be the latest one, is all :)

I think it's _extremely_ important to have accurate ChangeLog entries.
Personally, I would rather see a bug than have a bug fix (*esp* a
complicated one) without proper ChangeLogs.

The ChangeLog entry for the latest round of signals is:

	* fvwm/**: Applied updated signal handler patch by Chris Rankin

IMO this is really not acceptable.  At the very, very, very least there
should be a list of files affected by the changes!  And there _really_
should be a description of each change, too.

I'm not suggesting Dominik should add these, of course, I'm saying Chris
(and others sending patches) really need to provide these ChangeLog
entries when they send the patch.  If you use Emacs it's trivial to
write them once you get into the habit of "C-x 4 a" after each change.
Typically you don't send a diff for the ChangeLog, since it will almost
never apply cleanly anyway; typically you just cut and paste the
vertabim ChangeLog before the patch, like this:

1999-01-06  Paul D. Smith  <[email-masked]>

	* configure.in: Update to autoconf 2.13.
	Use new AC_FUNC_SELECT_ARGTYPES to get select() argtypes.

--- configure.in.tst    Mon Jan  4 12:14:50 1999
+++ configure.in        Wed Jan  6 11:53:23 1999
@@ -1,8 +1,8 @@
 dnl Process this file with autoconf to produce a configure script.
 dnl
-AC_PREREQ(2.12)
+AC_PREREQ(2.13)
 AC_INIT(fvwm/fvwm.c)
    ...

Note I'm not saying every minor one-line patch posted to this list needs
a ChangeLog entry; if it's simple enough or requires more hacking, the
person who commits it to CVS can add the entry themselves.

But I think we should make a firm(-ish :) policy that any larger-scale
patches, esp. those applied verbatim, _must_ have reasonable ChangeLog
entries, or they won't be accepted.

What do folks think of this?

PS. The reason I'm writing now is that fvwm-latest no longer compiles on
    SunOS, because events.c uses sig_atomic_t which isn't defined on
    SunOS.  I haven't heard about the details of how the new patches
    Chris supplied are supposed to work so I'm not sure how to autoconf
    them, although they obviously need some autoconf'ing... ChangeLog
    entries would greatly help me with this.

---

From: Dominik Vogt <[email-masked]>
Date: Fri, 8 Jan 1999 01:38:23 +0100

> +++ configure.in        Wed Jan  6 11:53:23 1999

You speak aloud what I was thinking. It's so much trouble 
looking through all these patches what all the changes in
all functions do! But don't be too harsh to the people
submitting patches. Most don't even know of the ChangeLog's.
Anyway I do request everybody to add ChangeLog entries.

Bye

Dominik

---

From: [email-masked] (Paul D. Smith)
Date: 08 Jan 1999 12:39:04 -0500

I've looked through the patches made with an eye towards autoconf'ing
them.

As best as I can tell, the USE_POSIX vs. USE_BSD is used only to choose
between using sigaction() vs signal()?

If so I can add autoconf tests for the sigaction function (and maybe
other things?  Can we assume struct sigaction, sigemptyset(), etc. if we
have sigaction()?), and use that instead.  I think we can assume that
all systems have signal() at least!  so that can be the default if
sigaction() doesn't exist.

Thoughts?

---

From: Chris Rankin <[email-masked]>
Date: Fri, 8 Jan 1999 19:49:52 -0500 (EST)

> between using sigaction() vs signal()?

Specifically, sigaction() and BSD-style signal()

> Can we assume struct sigaction, sigemptyset(), etc. if we
> have sigaction()?), and use that instead.

I think that having sigaction() implies all of the following are also present:

struct sigaction
sigset_t
sigemptyset()

if only because there is a sigset_t element within struct
sigaction. So autoconfing for HAVE_SIGACTION will work.

> I think we can assume that

Well, yes, sort-of, but SYSV signal() is subtly different from BSD
signal() which is why I tried to enforce BSD semantics. BSD semantics
are also a lot more intuitive and are closer to the way sigaction()
works by default.

Chris.