Closed Bug 380783 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: XPCOM, defect, minor)

defect
Not set
minor

Tracking

()

RESOLVED FIXED

People

(Reporter: Mook, Assigned: Mook)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Keywords: helpwanted
We have a good use for this, but I'm not sure when I'll have time to get to it.
Attached patch initial guess, under-tested (obsolete) — Splinter Review
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.
Attachment #268453 - Flags: review?(benjamin)
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.
Attachment #268453 - Flags: superreview?(dbaron)
Attachment #268453 - Flags: review?(benjamin)
Attachment #268453 - Flags: review+
This patch compiles just fine for us and works as expected. Thanks Mook!
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.
Attachment #268453 - Flags: superreview?(dbaron) → superreview-
Attached patch setIsVoid(PRBool) (obsolete) — Splinter Review
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.
Assignee: nobody → mook.moz+mozbz
Attachment #268453 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #273065 - Flags: superreview?
Attachment #273065 - Flags: review?
Attachment #273065 - Flags: superreview?(dbaron)
Attachment #273065 - Flags: superreview?
Attachment #273065 - Flags: review?(dbaron)
Attachment #273065 - Flags: review?
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
Attachment #273065 - Flags: superreview?(dbaron)
Attachment #273065 - Flags: superreview+
Attachment #273065 - Flags: review?(dbaron)
Attachment #273065 - Flags: review?(benjamin)
Attachment #273065 - Flags: review+
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.
Attachment #273065 - Flags: review?(benjamin)
Attachment #273065 - Flags: review+
Attachment #273065 - Flags: approval1.9?
Blocks: 324440
Comment on attachment 273065 [details] [diff] [review]
setIsVoid(PRBool)

a=bzbarsky
Attachment #273065 - Flags: approval1.9? → approval1.9+
Attachment #273065 - Attachment is obsolete: true
Attachment #275509 - Flags: review?(benjamin)
Attachment #275509 - Flags: review?(benjamin) → review+
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
Attachment #275509 - Attachment is obsolete: true
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
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?
The branch is API-frozen and it is not possible to land this patch on the branch.
(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.
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.
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?
You need to log in before you can comment on or make changes to this bug.