bug 928060 added static initializers due to global std::string variables in a header file

RESOLVED FIXED in mozilla30

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: froydnj, Assigned: jib)

Tracking

unspecified
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

Filing this as a separate bug so we don't lose track of it.
What's the objection here? Startup cost or something else?
(In reply to Eric Rescorla (:ekr) from comment #1)
> What's the objection here? Startup cost or something else?

Startup cost, code bloat, general objection to defining variables in a header file...
Converting them to enums seems reasonable, which should take care of this.

I suspect this will be addressed by Bug 906968.
froydnj and I discussed this on IM. We are going to convert them to const char[].

I don't think enums are a good idea.
I noticed the static constructors from a valgrind run; I think I count 10 constructors.
Now is a good time to fix this; I went with ekr's suggestion of string constants in
comment 4.

I do, however, think that jib's suggestion of enum variables would be better; ekr,
can you expound on why you felt strings were better here?  (I don't remember our
purported IRC conversation.)
Attachment #8366005 - Flags: review?(ekr)
Comment on attachment 8366005 [details] [diff] [review]
don't define std::string variables in nricectx.h

Review of attachment 8366005 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm

Honestly, I'm trying to remember why I preferred strings to enums.

Probably some combination of readability for debugging/logging
avoidance of multiple groups of potentially inconsistent semi-integers
(remember we are itnerfacing with C code).
Attachment #8366005 - Flags: review?(ekr) → review+
https://hg.mozilla.org/mozilla-central/rev/ad04260889bf
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Depends on: 970569
You need to log in before you can comment on or make changes to this bug.