105173 - DBM: mcom_db.h requires users to define HAVE_SYS_CDEFS_H and HAVE_SYS_BYTEORDER_H.

Status: RESOLVED FIXED

(SeaMonkey :: General, defect, P2)

Description

[Wan-Teh Chang]

This is the last problem in my attempt to make DBM (mozilla/dbm)
suitable for use by products other than the Mozilla client.

The exported header file mcom_db.h checks three autoconf-style
feature macros: HAVE_SYS_CDEFS_H, HAVE_SYS_BYTEORDER_H, and
HAVE_COMPAT_H.  This means that users of mcom_db.h needs to
define these HAVE_ macros in their build systems.

I'm wondering if you would be willing to consider the following
two proposals.

Proposal A: Revert to the original code, which tests the
platform macros.  Note that we only use platform macros in
mcom_db.h.  We still use the autoconf-style feature macros
in the private headers and .c files.

Proposal B: This proposal has three parts.
1. HAVE_SYS_CDEFS_H: Replace the macros defined in <sys/cdefs.h>
   or "cdefs.h" by their NSPR equivalents.  I believe the only
   things that mcom_db.h needs from <sys/cdefs.h> or "cdefs.h"
   are __BEGIN_DECLS and __END_DECLS, which can be replaced by
   PR_BEGIN_EXTERN_C and PR_END_EXTERN_C.

2. HAVE_SYS_BYTEORDER_H: Put the whole thing that deals with
   BYTE_ORDER, BIG_ENDIAN, and LITTLE_ENDIAN, including the
   test for HAVE_SYS_BYTEORDER_H, inside an
   #ifdef __DBINTERFACE_PRIVATE block because these are only
   needed when building DBM and not needed by DBM clients.

3. HAVE_COMPAT_H: Revert this to the original code, which
   tests for the macro SVR4.  (This is only used to distinguish
   between Solaris and SunOS 4.  Since we don't support
   SunOS 4, I don't really care about this one.)

I will attach two patches that show in concrete terms my
two proposals.  Please let me know what you think.

Comment 1

[Wan-Teh Chang]

Attachment: Proposal A: go back to the use of platform macros (in mcom_db.h only).

Comment 2

[Wan-Teh Chang]

Attachment: Proposal B: put the tests for HAVE_XXX_H inside #ifdef __DBINTERFACE_PRIVATE.

Comment 3

[Wan-Teh Chang]

Attachment: Proposal B: put the tests for HAVE_XXX_H inside #ifdef __DBINTERFACE_PRIVATE.

Comment 4

[Wan-Teh Chang]

I did verification build of mozilla0.9.5 with proposed
patch B (attachment 53933 [details] [diff] [review]) applied on Mac, Windows, and
Linux.

I would appreciate it if I could get a code review.

Comment 5

[Brendan Eich [:brendan]]

r=[e-mail]

Comment 6

[Wan-Teh Chang]

Proposed patch B (attachment 53933 [details] [diff] [review]) checked in.

Comment 7

[Axel Hecht]

bustage on solaris...

Index: mcom_db.h
===================================================================
RCS file: /cvsroot/mozilla/dbm/include/mcom_db.h,v
retrieving revision 3.31
diff -u -r3.31 mcom_db.h
--- mcom_db.h   2001/10/19 05:45:49     3.31
+++ mcom_db.h   2001/10/19 09:04:11
@@ -112,7 +112,7 @@
 #define BIG_ENDIAN      4321
 #define LITTLE_ENDIAN   1234            /* LSB first: i386, vax, all NT risc */
 
-#ifndef SVR4
+#ifdef SVR4
 /* compat.h is only in 4.1.3 machines. - dp */
 #include <compat.h>
 #endif

Comment 8

[Wan-Teh Chang]

I restored the test for HAVE_COMPAT_H for __sun platforms.
Apparently we no longer define SVR4 on Solaris.

I may test for __SVR4 instead, which is defined by all
compilers on Solaris SPARC.  I need to verify that it is
also defined by compilers on Solaris x86.

Do we have any SunOS 4.1.3 machine at Netscape?

Comment 9

[Chris McAfee]

I can't find any 4.1.3* sunos machines any more, the few
names I know about have all been upgraded.

Comment 10

[Wan-Teh Chang]

Attachment: Patch to test __SVR4 for Solaris.

Comment 11

[Wan-Teh Chang]

The Solaris bustage caused by my proposed patch B
exposed the fact that we are no longer defining
the SVR4 macro for Solaris.  However, some of the
DBM files still test the SVR4 macro.  Fortunately,
all of those tests for SVR4 are in dead code.

The patch I just attached fixed the SVR4 problem.
Some comments:
1. I am testing the __SVR4 macro (defined by gcc
   2.7.2.3 and 2.95.2, WorkShop 4.2, 5.0, and 6)
   instead of SVR4 (which is no longer defined by
   Mozilla's build system) and __svr4__ (defined
   by gcc but not by WorkShop).
2. For the inclusion of <compat.h>, I am testing
   for __SVR4 instead of HAVE_COMPAT_H.  The
   original Berkeley DB 1.85 comes with a header
   file named compat.h.  Berkeley DB 1.85 files
   always say #include <foo.h> and never say
   #include "foo.h", so I cannot tell whether
   #include <compat.h> means to include the
   standard header file compat.h or Berkeley DB's
   own compat.h.  I suspect that it means to include
   Berkeley DB's compat.h, although I can't find a
   SunOS 4.1.3 machine to verify my conjecture.
   Therefore, I am reverting the HAVE_COMPAT_H
   test to the __SVR4 test (was testing for SVR4
   originally), with dp's original comment.

cls, mcafee, could you review this patch (attachment
54603 [details] [diff] [review])?

Comment 12

[Wan-Teh Chang]

I just wanted to add that mozilla/include/platform.h
also tests the __SVR4 macro.

Comment 13

[Chris McAfee]

r=mcafee, I assume you have tested this.

Comment 14

[Wan-Teh Chang]

Chris McAfee wrote:
>
> r=mcafee, I assume you have tested this.

The tests for the SVR4 macro are all in dead code
(which is why people didn't notice they were broken),
so I can't test them, but the SVR4 to __SVR4 change
is simple and I have verified that all versions of
gcc and Solaris Workshop compilers for Solaris (both
SPARC and x86) define __SVR4.

The only thing that I can test is the HAVE_COMPAT_H
to __SVR4 change.  I have tested that in a Mozilla
0.9.5 build on Solaris 7 with both gcc 2.95.2 and
WorkShop 5.0.

Comment 15

[Wan-Teh Chang]

I checked in the __SVR4 patch.