210409 - xpcom exposes implementation details via header to embeddors

Status: RESOLVED INCOMPLETE

(Core :: XPCOM, defect)

Description

[hacker formerly known as [e-mail]]

Right now, in order to use xpcom, we're required to use mozilla-config.h so that
we have the proper defines needed for the compiler that xpcom was built against.
 In theory, xpcom should not be exposing these implementation details to the
users of xpcom.

The purpose of this bug is three-fold:
1) Move the xpcom specific defines into a separate xpcom-config header.
2) Further divide those defines into implementation specific and behavior
defining groups.  (Only xpcom should depend upon the first and everyone who uses
xpcom should depend upon the latter.)
3) Look at the viability of removing the dependency upon those defines for the
embeddors.

Comment 1

[hacker formerly known as [e-mail]]

Attachment: v1.0

This patch moves the following defines into xpcom-config.h:
CPP_THROW_NEW
HAVE_CPP_2BYTE_WCHAR_T
HAVE_CPP_ACCESS_CHANGING_USING
HAVE_CPP_AMBIGUITY_RESOLVING_USING
HAVE_CPP_BOOL
HAVE_CPP_DYNAMIC_CAST_TO_VOID_PTR
HAVE_CPP_EXPLICIT
HAVE_CPP_MODERN_SPECIALIZE_TEMPLATE_SYNTAX
HAVE_CPP_NAMESPACE_STD
HAVE_CPP_NEW_CASTS
HAVE_CPP_PARTIAL_SPECIALIZATION
HAVE_CPP_TROUBLE_COMPARING_TO_ZERO
HAVE_CPP_TYPENAME
HAVE_CPP_UNAMBIGUOUS_STD_NOTEQUAL
NEED_CPP_UNUSED_IMPLEMENTATIONS
NEW_H
HAVE_64BIT_OS
HAVE_DLFCN_H
HAVE_ICONV
HAVE_ICONV_WITH_CONST_INPUT
HAVE_MBRTOWC
HAVE_MMAP
HAVE_WCRTOMB

The last 7 are private defines which are wrapped inside a _IMPL_NS_COM ifdef. 
Right now, anything that uses xpcom should use nscore so it should pull in
xpcom-config.h as well.  I still think we should require every user of xpcom to
directly include this header.  We probably want to move the private headers to
a separate xpcom/private-config.h but that would require evey xpcom source file
to be modified to include that header.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 126390 [details] [diff] [review]
v1.0

+# xpcom-config.h is generated by configure
+EXPORTS		= xpcom-config.h

should be the SDK_EXPORTS define

I am not sure that this change is required:
ipc/ipcd/shared/src/ipcStringList.h

You should talk to darin about this one.

Comment 3

[Darin Fisher]

ipcStringList uses CPP_THROW_NEW, so if that is only found in xpcom-config.h,
then i guess it makes sense for ipcStringList.h to #include that.

Comment 4

[hacker formerly known as [e-mail]]

Attachment: v1.1

Added some comments to xpcom-config.h.in
Added check for getpagesize (used by lea-malloc)
Fixed -ldl check to search for both libdl & dlfcn.h to avoid some bad
assumptions.  Not sure why we explicitly check in xpcom when other places
assume libdl exists (some of those places are wrapped in XP_UNIX ifdef fwiw)

Comment 5

[Doug Turner (:dougt)]

Comment on attachment 126594 [details] [diff] [review]
v1.1

looks fine. r=dougt

Comment 6

[Brendan Eich [:brendan]]

Can we avoid dependencies on #ifdef'd _IMPL_* details by moving those to another
header file?

BTW, leading _ is reserved to the ISO C or C++ implementation.  Lots of headers
use nsFoo_h___ as the guard macro naming convention.

/be

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

nsFoo_h___ is also wrong, since any name containing two underscores is reserved
to the implementation.  To be more specific, C++ section 17.4.3.1.2 says:

  Certain sets of names and function signatures are always reserved to the
  implementation:

    * Each name that contains a double underscore ("__") or begins with an
      underscore followed by an uppercase letter (lex.key) is reserved to
      the implementation for any use. 

    * Each name that begins with an underscore is reserved to the
      implementation for use as a name in the global namespace.

Comment 8

[hacker formerly known as [e-mail]]

> Can we avoid dependencies on #ifdef'd _IMPL_* details by moving those to another
> header file?

Sure, but that's going to require every source file in xpcom & string (sans
xpcom/obsolete) to directly include the to-be generated xpcom-private.h.  

> BTW, leading _ is reserved to the ISO C or C++ implementation.  Lots of headers
> use nsFoo_h___ as the guard macro naming convention.

Open a separate bug on the header guard issue if there isn't one already.  We
have headers which use __NS_FOO_H__ (art_vpath.h), _NSFOOH_
(nsUsageArrayHelper.h) & FOO_H (jpeglib.h) as well.

Comment 9

[hacker formerly known as [e-mail]]

Attachment: v2.0

Split implementation defines into xpcom/xpcom-private.h and add includes only
where necessary.

Comment 10

[Doug Turner (:dougt)]

Comment on attachment 126846 [details] [diff] [review]
v2.0

I don't know if these are really private:

+/* Define if iconv() is available */
+#undef HAVE_ICONV
+
+/* Define if iconv() supports const input */
+#undef HAVE_ICONV_WITH_CONST_INPUT

I would guess, that this show only be allowed to build if the above is defined:

http://lxr.mozilla.org/seamonkey/source/intl/uconv/native/nsNativeUConvService.
cpp

what is that trailing char in xpcom/build/malloc.c?

Comment 11

[hacker formerly known as [e-mail]]

The trailing char is the ^L that's embedded into malloc.c.

Hrm.  nsNativeUConvService.cpp doesn't use either of those HAVE_ICONV* defines
even though it probably should.  It uses MOZ_USE_NATIVE_UCONV which is set by
--enable-native-uconv (yet another never tested feature).  The configure switch
should probably check for HAVE_ICONV and fail if it's not present.  They still
shouldn't be tree-afflicting defines.  We need a private header for gecko but
I'm not sure where to put it.  Maybe I'll just make one for intl.

Comment 12

[hacker formerly known as [e-mail]]

Attachment: v2.1

Same as above but adds a check to configure.in to stop if --enable-native-uconv
is set when iconv() support is not detected.

Comment 13

[Doug Turner (:dougt)]

Comment on attachment 126979 [details] [diff] [review]
v2.1

r=dougt

Comment 14

[alanjstr]

This got review+ back in 2003.  Whatever happened to it?