22479 2008-11-24 22:50:33 -0800 Consider renaming to 'deepCopy' the 'copy' methods that perform a deep copy 2008-11-26 00:33:58 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All All NEW P2 Normal --- 1 fishd webkit-unassigned ap ggaren oldest_to_newest 100162 0 fishd 2008-11-24 22:50:33 -0800 Consider renaming to 'deepCopy' the 'copy' methods that perform a deep copy This includes at least the copy methods on String, KURL, and SecurityOrigin. The intent here is to make the method name more self-documenting. 100164 1 ggaren 2008-11-24 22:53:28 -0800 Anders mentioned on IRC that "deepCopy" is overkill since "deep" is the default behavior of "copy." I researched a bit, and I don't think that's the case. "copy" in CSS, the DOM, HistoryItem, DocumentLoader, BindingURI, and StorageMap are not deep. ("copy" in JavaScriptCore is not always deep, either, but maybe that's not relevant here.) 100165 2 ggaren 2008-11-24 22:54:42 -0800 "copy" *is* deep for: String, StringImpl, SecurityOrigin, KURL. 100207 3 ap 2008-11-25 06:27:28 -0800 I think that StringImpl::deepCopy() would be a misnomer (and a comment in StringImpl.h is already slightly misleading). Also, moving an immutable object across threads doesn't necessarily involve making a deep copy - e.g. members that are ThreadSafeShared can be shallow-copied. 100270 4 ggaren 2008-11-25 13:44:08 -0800 > I think that StringImpl::deepCopy() would be a misnomer (and a comment in > StringImpl.h is already slightly misleading). Why do you think the comment is misleading? Because the reference count doesn't get copied? > Also, moving an immutable object > across threads doesn't necessarily involve making a deep copy - e.g. members > that are ThreadSafeShared can be shallow-copied. If the sole purpose of these functions is to move objects across threads, and they should optimize to copy ThreadSafeShared data in a shallow way, then a name like "threadSafeCopy" would be better. What do you think of that? 100284 5 ap 2008-11-25 14:07:15 -0800 (In reply to comment #4) > Why do you think the comment is misleading? Because the reference count doesn't > get copied? I think I was a bit too quick to criticize the comment. Maybe I'm just confused trying to understand why copy() is bad - shallow copy is the default one, so a method named copy() should be expected to do something different, shouldn't it? > If the sole purpose of these functions is to move objects across threads, and > they should optimize to copy ThreadSafeShared data in a shallow way, then a > name like "threadSafeCopy" would be better. What do you think of that? That seems to be rather wordy, especially for the just added String::substringCopy(). Worse, it doesn't add much clarity, as it doesn't describe what is thread safe (it sounds like it is the copy operation itself). And "thread safe" is a very bloated term. Copying objects across threads is the only reason, but not the only use case - the same function needs to be used when getting/setting values in global objects, such as caches, where names that talk about threads (like "detachFromThread()") would be confusing, too. The best I can think of is "copyOut()" - it's somewhat mysterious, but concise, and close to the meaning that it has in kernel programming. But we won't have a matching "copyIn()". Or maybe we could just keep "copy()". 100343 6 ggaren 2008-11-25 17:13:21 -0800 > Maybe I'm just confused trying to understand why copy() is bad - shallow copy > is the default one, so a method named copy() should be expected to do something > different, shouldn't it? There's disagreement about what the default is. Anders thought that deep copy was the default. Turns out, neither is the default. WebCore is split about 50 / 50, and nothing is the default. I think it's important to specify the type of copy in the name because, in a world where there is no default, "copy" is ambiguous. The proof is in the pudding: Each use of copy is annotated by a comment that says "this is a deep copy." That's bad programming 101. If "copy" means "deepCopy", and needs a comment to say so, we should just rename it to "deepCopy," and get rid of the comment. I think StringImpl::copy is indeed a "deep" copy. Based on your comments, it seems like it might be invalid to try to optimize copy by not copying ThreadSafeShared data, so I think it is the case that all instances of "copy" that claim to perform a "deep" copy do so, and need to do so. > The best I can think of is "copyOut()" - it's somewhat mysterious, but concise, > and close to the meaning that it has in kernel programming. But we won't have a > matching "copyIn()". Or maybe we could just keep "copy()". You're oversimplifying. If we do nothing, we keep a function named "copy()" where each use is annotated by a comment that says "this is a deep copy." That seems like the worst of all possible worlds. 100384 7 ap 2008-11-26 00:33:58 -0800 (In reply to comment #6) > Based on your comments, itseems like it might be invalid to > try to optimize copy by not copying ThreadSafeShared data I didn't mean this, and I think that this would be correct for immutable ThreadSafeShared data - what do you mean?