280872 - Do not include <stdlib.h> in prmem.h

Status: RESOLVED WONTFIX

(NSPR :: NSPR, defect)

Description

[Hans-Andreas Engel]

Note that prmem.h "wraps up malloc, free, calloc, realloc so they are already
thread safe."  This implies that users of prmem.h are generally not interested
in the memory functions declared in <stdlib.h>.  For other functions of
<stdlib.h>, this header should be included explicitly (at least not via prmem.h).

Thus, "#include <stdlib.h>" should be removed from prmem.h.

Since prmem.h is used very often, this change should slightly decrease compile time.

Comment 1

[Hans-Andreas Engel]

Attachment: Remove "#include <stdlib.h>" from prmem.h

Also, "#include <stdlib.h>" is explicitly added where required.

Comment 2

[Wan-Teh Chang]

You are right.  prmem.h doesn't need to include
<stdlib.h>.  The only thing it needs is the NULL
macro, which is defined in <stddef.h>.  And
since prtypes.h already includes <stddef.h>,
the "#include <stddef.h>" in prmem.h can also be
removed.

However, the removal of "#include <stdlib.h>"
breaks source compatibilty, as your patch shows.
There are many products outside the Mozilla
project that use NSPR.  So the benefit of
removing #include <stdlib.h> from prmem.h is not
worth the trouble of all of them fixing their
files to add the necessary #include <stdlib.h>

I'll be happy to remove the #include <stddef.h>
though.

Comment 3

[Wan-Teh Chang]

Comment on attachment 173229 [details] [diff] [review]
Remove "#include <stdlib.h>" from prmem.h

>Index: nsprpub/pr/src/md/mac/macdll.c

>Index: nsprpub/pr/src/md/mac/macio.c

>Index: nsprpub/pr/src/md/mac/macthr.c

>Index: nsprpub/pr/src/md/windows/w16mem.c

>Index: nsprpub/pr/src/md/windows/w16stdio.c

These are dead files, for Mac OS Classic
and 16-bit Windows.  I'm curious as to how
you determined that these files need to
include <stdlib.h> explicitly.

Comment 4

[Hans-Andreas Engel]

> I'm curious as to how you determined that these files need to
> include <stdlib.h> explicitly.

I searched through all platform-specific files and identified the ones which use
malloc etc.  Then, I checked all these (~150) files by hand to see if they
include <stdlib.h> either directly or via some other header file (excluding
prmem.h>).  Otherwise, I included <stdlib.h> explicitly.

This took quite some time; but I wanted to reduce the probability that the
proposed change breaks something.

Comment 5

[Hans-Andreas Engel]

For consistent memory allocation, one should either
  a) include "prmem.h" and use PR_MALLOC() or
  b) include <stdlib.h> and use malloc().

It seems to me that it is a bad style to
  c) include "prmem.h" and use malloc()
and that this should not be encouraged by NSPR.

I agree that the proposed change could break some builds.  If it does, it would
just lead to a compiler error such as
    myFile.c++:42: error: `malloc' undeclared.
It would then be very easy (possibly after checking malloc's man page) to
identify that <stdlib.h> needs to be included in myFile.c++.  So I think that
even the worst-case scenario is not too disturbing.

However, with your decade-long experience on NSPR, I of course trust that you
can better assess the possible damage of the change proposed here.

Comment 6

[Wan-Teh Chang]

I agree with everything you said, but I
want to give our users a very smooth
upgrade to new NSPR releases.  That's
our promise, and we try very hard to
keep it.  The #include <stdlib.h> is a
*minor* problem, and the inconvenience
caused by its removal isn't worth it.

(By the way, looking at prmem.h more,
I suspect the #include <stdlib.h> may
even have been intentional.  But it's
a mistake to put it there.)

I also want to point out that it may
even introduce bugs on 64-bit platforms.
Suppose we have the following C code:

    char *buf;

    buf = (char *) malloc(100);

If malloc is not declared, most C compilers
may warn about it but will happily accept
it as a function returning int.  On a 64-bit
platform, an int is 32 bits, so the return
value of malloc (a 64-bit pointer) may be
truncated to 32 bits.  If our users do not
review compiler warnings after an NSPR upgrade,
this bug may be introduced.

I thank you for spending the effort to prepare
a good patch.  And I hope you understand my
decision and it won't discourage you.

Comment 7

[Wan-Teh Chang]

I forgot to mention that I've removed
the #include <stddef.h> from "prmem.h"
on the tip of NSPR (NSPR 4.6).  This
change will soon show up in the
version of NSPR used by Mozilla.

Comment 8

[Hans-Andreas Engel]

> I agree with everything you said, but I want to give
> our users a very smooth upgrade to new NSPR releases.

This is a noble task which I do not want to spoil.

> (...) (By the way, looking at prmem.h more, I suspect the #include 
> <stdlib.h> may even have been intentional.  But it's a mistake to > put it there.)

Well, it seems that we now have to live with this mistake; especially given your
argument about 64bit C compilers.

Thank you for the clarifications.