Warnings from including string.h

RESOLVED WONTFIX

Status

()

Core
String
P3
normal
RESOLVED WONTFIX
19 years ago
15 years ago

People

(Reporter: Akkana Peck, Assigned: Scott Collins)

Tracking

({pp})

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

19 years ago
On Linux, including <string.h> tends to cause lots of warnings because it
defines index (another name for strchr), and lots of people like to use that
name for their variables.  The warning is:
    warning: declaration of `index' shadows global declaration


We're including string.h in the following header files exported to
dist/include.  In most cases there's no need to include it in the header file at
all; it should be included in the .cpp file instead.

extcache.h
interpreter.h
jsarena.h
nsCRT.h
nsID.h
nsTarget.h
seccomon.h

I suspect the one causing the most problems (included in the most places) is
nsID.h, with nsCRT.h second.  nsCRT.h in its current form does need the
inclusion because it has inlines to call various system functions.

In order to cut down on warnings, we would need to decide whether to stop
including string.h unless necessary, or not name variables "index".

Updated

19 years ago
Assignee: rickg → dp
QA Contact: rickg → dp

Comment 1

19 years ago
scc is new owner of string stuff, but I think the bug here is the gratuitous
nested include.  Only if the header itself required a type size or name from
string.h should it include that file.  I'm reassigning to dp and cc'ing scc and
warren, the XPCOM troika who can fix most of the offending .h files you found.
Either other bugs should be filed, or this bug should be passed around, till all
gratuitous <string.h> includes have been removed.

I'm removing the gratuitous include of <string.h> from jsarena.h, which
shouldn't have been there, and wasn't back in the day (when it was prarena.h).
Fur stop me if I'm doing wrong.

/be

Comment 2

19 years ago
Don't forget to check it in on SM140_BRANCH too.

Comment 3

19 years ago
Ugh! what is SM140_BRANCH

Updated

19 years ago
Summary: Warnings from including string.h → [PP]Warnings from including string.h

Updated

19 years ago
Target Milestone: M10

Updated

19 years ago
Status: NEW → ASSIGNED

Updated

19 years ago
Target Milestone: M10 → M11
I've got a build here without <string.h> in nsID.h, and all went well.  I'm
going to check it in shortly and see how many of these warnings are left.

Updated

19 years ago
Whiteboard: [HELP WANTED]

Comment 5

19 years ago
Shaver you want to take this bug over ?

Updated

19 years ago
Whiteboard: [HELP WANTED]
Target Milestone: M11 → M15

Comment 6

19 years ago
This is cleanup. Moving to post beta.

Updated

18 years ago
Keywords: pp

Updated

18 years ago
Summary: [PP]Warnings from including string.h → Warnings from including string.h

Updated

18 years ago
Target Milestone: M15 → M16

Comment 7

18 years ago
Scott ?
Assignee: dp → scc
Status: ASSIGNED → NEW

Comment 8

18 years ago
Two points: (1) |index| is likely to live wherever |memchr| lives (meaning 
<string.h>, or perhaps <cstring> on most systems), and because |memchr| is used 
in inline functions in .h files, e.g., "nsCharTraits.h", there will be trouble; 
(2) |index| is not, AFAIK, part of the C or C++ standard; it is a platform 
extension synonym for |memchr| in a few places.  Perhaps the right way to fix 
this is to fix the header file?  Get better headers?

Someday, of course, we'll be using namespaces and problems like this will less 
frequent.  How bad is it to fix the header file and remove the non-standard 
global namespace polluting declaration?
Status: NEW → ASSIGNED
Target Milestone: M16 → M20
(Reporter)

Comment 9

18 years ago
How bad is it to tell developers that they all need to go edit their system
include files to get rid of the reference to index (which some other program
might actually use?)  I don't think most people will willing to do that.
If we could minimize the number of places that include <string.h>, then we could 
do this in all those places:

#define index _pre_ansi_invasive_index
#include <string.h>
#undef index

and (no one linking with Mozilla code should ever call index, right?) we'd be 
free of those annoying warnings.  Or am I missing something?

/be
(Reporter)

Comment 11

18 years ago
Yes, that would be a fine solution, and in fact minimizing the number of places
that include <string.h> was my intent in filing this bug in the first place,
since few files should need to include it (they should usually be using nsCRT.h
or nsString.h or some other utility).
Sorry if obvious: don't make jsarena.h include any xpcom header file, or any 
header outside of js/src -- the JS engine must stand alone.  So ifdef ugliness 
will have to infest at least two (three if interpreter.h is Java) places.

/be

Comment 13

18 years ago
mass re-assigning to my new bugzilla account
Assignee: scc → scc
Status: ASSIGNED → NEW
(Assignee)

Updated

18 years ago
Status: NEW → ASSIGNED

Comment 14

17 years ago
dp is no longer @netscape.com
changing qa contact to default for this component
QA Contact: dp → scc

Comment 15

16 years ago
[bugzilla component to be deleted]
Status: ASSIGNED → NEW
Component: XP Utilities → String
QA Contact: scc → jaggernaut
(Assignee)

Comment 16

15 years ago
apparently, no one has actually cared about this bug for 2 years
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.