Closed
Bug 1431261
Opened 6 years ago
Closed 6 years ago
nsDependentTString needs a copy constructor
Categories
(Core :: XPCOM, enhancement)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla59
Tracking | Status | |
---|---|---|
firefox59 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
Details
Attachments
(1 file, 1 obsolete file)
3.11 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•6 years ago
|
||
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 | ||
Updated•6 years ago
|
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)
Assignee | ||
Comment 3•6 years ago
|
||
(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)
Assignee | ||
Comment 4•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8943790 -
Attachment is obsolete: true
Attachment #8943790 -
Flags: review?(dbaron)
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
(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+
Comment 10•6 years ago
|
||
Pushed by erahm@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ee16cc590134 Add nsTDependentString copy constructor. r=dbaron
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #9) > Maybe also worth asserting (EXPECT_TRUE) that tmp.IsEmpty() here? Updated locally.
Comment 12•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ee16cc590134
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•3 years ago
|
Component: String → XPCOM
You need to log in
before you can comment on or make changes to this bug.
Description
•