380783 - nsStringAPI.h: no equivalent of IsVoid (tell if string is null)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[:Mook]

in nsStringAPI.h (external strings), there is no equivalent of nsAString::IsVoid(); this makes it impossible to tell when JS has passed null instead of an empty string.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We have a good use for this, but I'm not sure when I'll have time to get to it.

Comment 2

[:Mook]

Attachment: initial guess, under-tested

Initial stab at a patch.  I don't like this very much yet, because
- I'm adding 4 functions to be exposed, and they'd pretty much have to be frozen.  I don't really like that.
- Haven't tested with internal/external strings (i.e. have one side set IsVoid and the other check)
- The string API tests I wrote sucks; mixing stuff and having in general largish functions.  And not testing nsACString yet.
- This isn't really an area of code I'm that familiar with.

Better API design, or even a smackdown saying this whole bug sucks, welcome.  Setting r? to get feedback, really.

Comment 3

[Benjamin Smedberg]

Comment on attachment 268453 [details] [diff] [review]
initial guess, under-tested

FWIW all the linkage and whatnot looks correct. I have mixed emotions about exposing void strings through the frozen API in general because I don't like the concept, but I think we're stuck with it for the 1.9 series and this will all be replaced/gone in moz2.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

This patch compiles just fine for us and works as expected. Thanks Mook!

Comment 5

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

Comment on attachment 268453 [details] [diff] [review]
initial guess, under-tested

I hate the concept as well, but I guess I'm ok with exposing it, since it's an important part of some of the C++ DOM bindings.

However, I have one issue with the patch.  I think you should pass the boolean flag for SetIsVoid through the frozen API rather than simulating it.  That seems much cleaner than making assumptions about what SetIsVoid(PR_FALSE) does -- I don't see why it should do anything different from what it does in the unfrozen API -- and having it be different makes it hard to convert code.  So I think you should pass the boolean through rather than doing stuff like this:

>+nsAString::SetIsVoid(PRBool val)
>+{
>+  if (val)
>+    NS_StringSetIsVoid(*this);
>+  else
>+    if (IsVoid())
>+      Truncate();

Other than that, I'm fine with this; sorry for the delay here.

Comment 6

[:Mook]

Attachment: setIsVoid(PRBool)

My thinking was to explicitly force external API to not depend on whether unsetting voidness would let you get back the string data that was there before it was set as void.  But I guess this would work too.

Comment 7

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

Comment on attachment 273065 [details] [diff] [review]
setIsVoid(PRBool)

r+sr=dbaron, but you should probably double-check that bsmedberg is ok with the change I suggested

Comment 8

[Benjamin Smedberg]

Comment on attachment 273065 [details] [diff] [review]
setIsVoid(PRBool)

>Index: xpcom/glue/nsStringAPI.h


>+  NS_HIDDEN_(void) SetIsVoid(PRBool);
>+  NS_HIDDEN_(PRBool) IsVoid() const;

Please inline these implementations here and below.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 273065 [details] [diff] [review]
setIsVoid(PRBool)

a=bzbarsky

Comment 10

[Mook (songbird dead account)]

Attachment: inlined things, please rs and skim to make sure I didn't screw up

Comment 11

[:Mook]

Attachment: re-generated patch

The previous patch apparently didn't want to apply, so I re-generated it.  Should have no changes from the last patch, so r/sr/a should be carried over

Comment 12

[Phil Ringnalda (:philor)]

xpcom/build/nsXPCOMPrivate.h 1.46
xpcom/build/nsXPCOMStrings.cpp 1.4
xpcom/glue/nsStringAPI.h 3.13
xpcom/glue/standalone/nsXPCOMGlue.cpp 1.50
xpcom/string/public/nsXPCOMStrings.h 1.3
xpcom/stub/nsXPComStub.cpp 1.13
xpcom/tests/Makefile.in 1.100
xpcom/tests/TestStringAPI.cpp 1.1

Comment 13

[Simon Paquet [:sipaq]]

Mook, we (Calendar Project) would really like to have this patch on the 1.8 branch as well, as this is the only issue that's currently blocking us from using frozen linkage on the branch and the trunk.

Would it be possible to get this on the 1.8 branch as well?

Comment 14

[Benjamin Smedberg]

The branch is API-frozen and it is not possible to land this patch on the branch.

Comment 15

[Daniel Boelzle [:dbo]]

(In reply to comment #14)
Just being curious: Why is that impossible? Adding symbols doesn't break compatibility. We've been adding symbols to our export mapfiles in UNO (OpenOffice's component model) for years, and haven't broken old component libraries.

Comment 16

[Benjamin Smedberg]

To add symbols to XPCOM you have to modify the standalone glue map at http://mxr.mozilla.org/mozilla/source/xpcom/build/nsXPCOMPrivate.h#192 which contains lots of extra entries between 1.8 and 1.9... the glue code would not deal with missing entries in the middle of this table gracefully.

Comment 17

[Daniel Boelzle [:dbo]]

Excuse my ignorance, but why is the 1.9 section already stable? Why couldn't the IsVoid entries be moved to the end of the 1.8 section?