235041 - Add nsAdoptingString helper class

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Johnny Stenback (:jst)]

A helper string class that adopts alloced string buffer on construction and
frees it on destruction would make it easier to deal with code in our mixed
string object/raw string buffer world, for instance methods that return malloced
string data and expect the callers to free it, could return an nsAdoptingString
by value and leave the memory management up to the string, in stead of expecting
the callers to get it right.

Patch coming up (per irc discussion with darin).

Comment 1

[Johnny Stenback (:jst)]

Attachment: Implement nsAdoptingString.

This is all inline, so hardly any codesize penalties...

Comment 2

[Johnny Stenback (:jst)]

Attachment: Updated fix based on discussion with darin.

Comment 3

[Johnny Stenback (:jst)]

The comment above nsTAdoptString in the patch talks about the wrong string
classes. Fixed locally.

Comment 4

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

Comment on attachment 141845 [details] [diff] [review]
Updated fix based on discussion with darin.

>+      self_type& operator=( const self_type& str )
>+        {
>+          Adopt(str.mData, str.mLength);
>+
>+          self_type& mutable_str(NS_CONST_CAST(self_type&, str));
>+
>+          // Make str forget the buffer we just took ownership of.
>+          new (&mutable_str) self_type();

I'd prefer:

  self_type* mutable_str = NS_CONST_CAST(self_type*, &str));
  new (mutable_str) self_type();

since I dislike mixing casts and references like that.

r=dbaron

Comment 5

[Johnny Stenback (:jst)]

Fixed.

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why not make operator= take a non-const argument? That const-cast seems
dangerous. What if someone passes an nsAdoptingString that really *is* const?

Comment 7

[Johnny Stenback (:jst)]

No matter where the nsAdoptingString that's assigned into an nsAdoptingString
comes from, I don't want it to not give up its buffer, even if it's const. Keep
in mind that this is not a class that's supposed to be used as a type any other
place than as a return type from functions, so what really matters is that the
copy constructor does the right thing, since that *might* be called when
returning these things by value (and the copy ctor takes a const argument, so
therefore operator= needs to take a const too, in this case).

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My idea was to make the copy ctor and the operator= take a non-const argument.
Like we do in nsAutoPtr (which btw is very similar in functionality).

Unfortunatly i'm not sure if this can be made in a way that makes all compilers
happy. There are some portability issues with nsAutoPtr that would probably
appear here too.

Comment 9

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

having a copy-constructor taking non-const should prevent the compiler from
auto-generating one taking const.