Closed Bug 1431261 Opened 6 years ago Closed 6 years ago

nsDependentTString needs a copy constructor

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: erahm, Assigned: erahm)

Details

Attachments

(1 file, 1 obsolete file)

As-is if you make a copy of an `nsDependentTString` it will make a heap allocation rather than rebind to the static buffer, ie the following fails:

>  nsDependentCString foo(nsDependentCString("foo"));
>  EXPECT_FALSE(foo.GetDataFlags() & StringDataFlags::SHARED);

The copy constructor can probably just call the substring constructor [1].

[1] https://searchfox.org/mozilla-central/rev/48cbb200aa027a0a379b6004b6196a167344b865/xpcom/string/nsTDependentString.h#96
This adds a copy constructor that uses Rebind to preserve the reference to
static data rather allocate a new shared buffer.
Attachment #8943790 - Flags: review?(dbaron)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
So I'm a little hesitant about exposing the operator= given the potential for lifetime issues that it might expose callers to.    What would happen if it were = delete instead?  Would there be C++ pitfalls where the operator is used implicitly?  Or do you think there are cases where it's particularly useful?

Also, the "XXX are you sure??" comment should probably go.
Flags: needinfo?(erahm)
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #2)
> So I'm a little hesitant about exposing the operator= given the potential
> for lifetime issues that it might expose callers to.    What would happen if
> it were = delete instead?  Would there be C++ pitfalls where the operator is
> used implicitly?  Or do you think there are cases where it's particularly
> useful?

Can you elaborate on the lifetime concerns that you have for operator=, but not for the copy constructor? It's operator= only on self type, so it won't work for other string types, ie:

> nsCString foo = nsDependentCString("foo"); // makes copy, shared buffer
> nsDependentCString bar = nsDependentCString("bar"); // no copy, uses same static buffer
> nsDependentCString baz = nsCString("baz"); // shouldn't compile

As far as whether it's useful...it's hard to think of a use case, but maybe:

> nsDependentString foo("http://bar.baz.com");
> // decide to strip off http://
> foo = nsDependentCString(foo, 7);

Arguably that could be:

> foo.Rebind(foo, 7);

So it probably wouldn't be the end of the world. I'll see if we actually use the implicit one anywhere currently.

> Also, the "XXX are you sure??" comment should probably go.

Hah, I didn't want be the one that said it was okay :) But sure I'll remove it.
Flags: needinfo?(erahm) → needinfo?(dbaron)
FWIW we compile fine with `operator=(...) = delete` -- after I removed my gtest of course!
I think the implicit destructor is pretty clearly OK -- it's the ones your changing that the comment was about.

Regarding concerns with operator= -- it might encourage people to use nsDependent[C]String as an out-param and assign to it; that's reasonably likely to be bad.  The question is then where it would be useful...

I think the example you give should probably use StringTail or other Substring variants (though StringTail should preserve the TERMINATED flag and String vs. Substring type... though I didn't check that it does!)
Flags: needinfo?(dbaron)
This adds a copy constructor that uses Rebind to preserve the reference to
static data rather allocate a new shared buffer.
Attachment #8943806 - Flags: review?(dbaron)
Attachment #8943790 - Attachment is obsolete: true
Attachment #8943790 - Flags: review?(dbaron)
(In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment #3)
> As far as whether it's useful...it's hard to think of a use case, but maybe:
> 
> > nsDependentString foo("http://bar.baz.com");
> > // decide to strip off http://
> > foo = nsDependentCString(foo, 7);
> 
> Arguably that could be:
> 
> > foo.Rebind(foo, 7);

nsTDependentString must be zero-terminated. So if we share the buffer, you have to use nsTDependentSubstring.
(In reply to Masatoshi Kimura [:emk] from comment #7)
> (In reply to Eric Rahm [:erahm] (please no mozreview requests) from comment
> #3)
> > As far as whether it's useful...it's hard to think of a use case, but maybe:
> > 
> > > nsDependentString foo("http://bar.baz.com");
> > > // decide to strip off http://
> > > foo = nsDependentCString(foo, 7);
> > 
> > Arguably that could be:
> > 
> > > foo.Rebind(foo, 7);
> 
> nsTDependentString must be zero-terminated. So if we share the buffer, you
> have to use nsTDependentSubstring.

In that example '7' is the starting index [1], so it will still be zero-terminated.

[1] https://searchfox.org/mozilla-central/rev/2031c0f517185b2bd0e8f6f92f9491b3410c1f7f/xpcom/string/nsTDependentString.cpp#19-37
Comment on attachment 8943806 [details] [diff] [review]
Add nsTDependentString copy constructor

>+    // Test move ctor.
>+    nsDependentCString tmp("foo");
>+    auto data = tmp.Data();
>+    nsDependentCString foo(mozilla::Move(tmp));
>+    // Neither string should be using a shared buffer.
>+    EXPECT_FALSE(tmp.GetDataFlags() & DataFlags::SHARED);
>+    EXPECT_FALSE(foo.GetDataFlags() & DataFlags::SHARED);
>+    // First string should be reset, the second should be pointing to the
>+    // original buffer.
>+    EXPECT_NE(data, tmp.Data());
>+    EXPECT_EQ(data, foo.Data());

Maybe also worth asserting (EXPECT_TRUE) that tmp.IsEmpty() here?


r=dbaron
Attachment #8943806 - Flags: review?(dbaron) → review+
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee16cc590134
Add nsTDependentString copy constructor. r=dbaron
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #9) 
> Maybe also worth asserting (EXPECT_TRUE) that tmp.IsEmpty() here?

Updated locally.
https://hg.mozilla.org/mozilla-central/rev/ee16cc590134
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.