[Xastir-dev] acinclude.m4:XASTIR_DETECT_DEVICES

[Jack Twilley]

WARNING: Unsanitized content follows.
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

>>>>> "Curt" == Curt Mills <archer at eskimo.com> writes:

Jack> No, that's a bad idea.  It goes against the grain of autoconf
Jack> and good software practices.  The code is very simple as it is:
Jack> Test for the presence of /dev/ttyS0 and use it if it's there.
Jack> Test for the presence of another popular serial file name and
Jack> use *that* if it's there.  I don't see any gain in changing it
Jack> in any way other than adding in future popular serial file
Jack> names.

Curt> I'm trying to wrap my head around the "bad idea" and "against
Curt> good software practices" comments.

Curt> It _is_ good practice to set default values for variables, then
Curt> set them to other values if special conditions are found.  It
Curt> saves you from explicitly checking for each and every
Curt> possibility.  

Actually, it's not good practice to set valid default values for
variables when attempting to identify current capabilities.  By
setting the defaults to reasonable values, you make it impossible to
detect certain error conditions and you increase the debugging required

Curt> That's what I'm suggesting doing.  It makes our code shorter.
Curt> Something like this:

Curt> AC_DEFUN([XASTIR_DETECT_DEVICES],
Curt> [ 
Curt> AC_MSG_CHECKING([for devices])
Curt> ac_tnc_port=/dev/ttyS0
Curt> ac_gps_port=/dev/ttyS1
Curt> if test -d /proc/registry ; then
Curt> elif test -c /dev/cuaa0 ; then 
Curt> ac_tnc_port=/dev/cuaa0 
Curt> ac_gps_port=/dev/cuaa1 
Curt> elif test -c /dev/cua/a ; then 
Curt> ac_tnc_port=/dev/cua/a
Curt> ac_gps_port=/dev/cua/b 
Curt> fi

Curt> AC_DEFINE_UNQUOTED([TNC_PORT], "$ac_tnc_port", [Default TNC port.])
Curt> AC_DEFINE_UNQUOTED([GPS_PORT], "$ac_gps_port", [Default GPS port.])  
Curt> AC_MSG_RESULT(found $ac_tnc_port and $ac_gps_port)
Curt> ])

Run this code on an imaginary machine where the following files are
not found on the system: com1, /dev/ttyS0, /dev/cuaa0, /dev/cua/a.
Tell me what gets displayed on the user's screen, and tell me what
variables are set.  Now run the existing code on that imaginary
machine, and tell me what happens.  I wrote the code to have bad
defaults so that failing all the way through the tests would show that
we had discovered a new machine that matched the imaginary machine's
constraints as shown above, and that the simplest and most correct way
to resolve this would be by adding another test at the foot of the
function.

If you want to reduce line count in xastir, there are many, many
places where it is more appropriate.  Look for all those lines
bracketed with "#if 0" and "#endif" for starters.  Trace through the
obsolete debugging code.  That will generate far more return on
investment than being penny-wise and pound-foolish in this function.

Curt> We may be able to get rid of the test for /proc/registry if the
Curt> tests for /dev/cuaa0 and /dev/cua/a when run on Cygwin/WinXP
Curt> don't cause a hang.  That hang is the whole reason for changing
Curt> this function.

If the hang is the whole reason, then we should only worry about
changing that part of the function, and not consider redesigning the
entire function without understanding the purpose behind its design.

Jack.
- -- 
Jack Twilley
jmt at twilley dot org
http colon slash slash www dot twilley dot org slash tilde jmt slash
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.4 (FreeBSD)

iD8DBQFAYLvgGPFSfAB/ezgRAlHbAKDwlC6G9+Z5kqev3MpX9D6JmdeO1ACeKXYt
whN6QJFoHOn9sIk0Uq9wE/Q=
=EX0I
-----END PGP SIGNATURE-----