Bug 31020 – String::crossThreadString is badly named.

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: Platform

[David Levin]

It makes it sound like the returned string may be used on more than one thread.

The fix is to rename this to "copy".

[David Levin]

Created attachment 42325 [details]
Proposed fix.

[David Levin]

This part is only for the crossThreadString method.

One issue: Should the copy method in String not be const anymore? (The const seems to imply that it is threadsafe like the length method but the copy method isn't threadsafe.)

There needs to be another patch to change the threadsafeCopy method to something else (which I haven't yet figured out a good name for).

[Oliver Hunt]

Comment on attachment 42325 [details]
Proposed fix.

I'm unsure about this name -- it doesn't seem to imply thread safety except by implying that it's a "real" copy, eg. implying that it's slow :-/

--Oliver

[Darin Adler]

Comment on attachment 42325 [details]
Proposed fix.

I understand that crossThreadString is a poor name. But copy doesn't seem to be better. Since a StringImpl is immutable it makes little sense to talk about copying a handle to one. I think the name has to say *something* about threading or at least something that mentions the storage or reference count. Maybe createDetachedCopy or deepCopy would be best?

[David Levin]

Comment on attachment 42325 [details]
Proposed fix.

Marking r- to move out of review queue per bug comments about a better name being needed.

Must go off and brainstorm.

[David Levin]

I've brainstormed a lot of potential names below:

  createForAnotherThread

  instanceForAnotherThread
  getInstanceForAnotherThread
  createInstanceForAnotherThread

  getReferenceForAnotherThread
  referenceForAnotherThread
  createReferenceForAnotherThread

  createNewInstance

  createDuplicateInstance


Personally, I like createDuplicateInstance because it doesn't imply the slowness of "copy" but yet seemed to indicate that there is a new instance with the same information which is what you need for another thread.

Is everyone (who wants to give an opinion) fine with "createDuplicateInstance"?

[Dmitry Titov]

createDuplicateInstance sounds good.

How about simply "clone"? It is usually the same as 'deepCopy', but hopefully without mandatory slowness.

[Darin Adler]

I’d like to help.

Could someone briefly review here when code needs to call this function and how it should be used? A quick how-to paragraph. I’m hoping that some of the words in that explanation could provide the inspiration for an even-better name.

[David Levin]

(In reply to comment #8)
> I’d like to help.
> 
> Could someone briefly review here when code needs to call this function and how
> it should be used? A quick how-to paragraph. I’m hoping that some of the words
> in that explanation could provide the inspiration for an even-better name.

The method should be called when code wants a string with the same contents for use on another thread.

It returns a duplicate of the string that is safe to pass to another thread. It attempts to do this efficiently by only doing a real copy for "small" strings. For other strings, it makes the string switch to a (relatively efficient) threadsafe ref counting mechanism for the underlying buffer.

Due to the way this method works, it is not threadsafe (when it does the switch to the threadsafe ref counting mechanism) unlike a read-only copy method would be., so the "const" attribute for the method should probably be removed to make this more clear. (I've been strongly considering this for another patch.)

[David Levin]

This is being cleaned up in 70698.

*** This bug has been marked as a duplicate of bug 70698 ***