If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsAString.h's kNotFound conflicts with LevelDB

NEW
Unassigned

Status

()

Core
String
--
minor
6 years ago
a month ago

People

(Reporter: mcpherrin, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
I am working on integrating LevelDB (bug 679852).

The "#define kNotFound -1" conflicts with an element of an enum in LevelDB.

If nsAString.h is #included before any LevelDB headers, at best we get a compile error (Because enum Code { ..., -1 = 1, ...}; is invalid syntax).

At worst, somebody in the leveldb:: namespace who uses kNotFound compares against -1 instead of 1, leading to incorrect behaviour.

Correct use of #undef kNotFound before #including LevelDB headers avoids this problem, however it may lead to future problems.

One possibility is changing nsAString.h. That's what this bug is for.
Looks like we could probably change that to const PRInt32 kNotFound = -1;
Or even to a static member on nsAString, and fix our consumers.
(Reporter)

Comment 3

6 years ago
Created attachment 574115 [details] [diff] [review]
Global constant kNotFound

Seems to build on Linux, at least. Pushing to try to find out what happens.

Comment 4

6 years ago
Try run for 7b687eed8c27 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=7b687eed8c27
Results (out of 209 total builds):
    success: 190
    warnings: 18
    failure: 1
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mmcpherrin@mozilla.com-7b687eed8c27

Comment 5

6 years ago
I've been working on a patch to make nsAString::kNotFound instead of a global, which I think is preferable but requires some treewide changes.
(Reporter)

Updated

6 years ago
Blocks: 679852

Comment 6

5 years ago
I'm not going to have the time to finish what I started here. If there's a simpler patch and this is still an issue we can probably take that.
You need to log in before you can comment on or make changes to this bug.