llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
29.11k stars 12.01k forks source link

copy is only important at the start of a method name #8496

Closed davidchisnall closed 13 years ago

davidchisnall commented 14 years ago
Bugzilla Link 8124
Resolution FIXED
Resolved on Dec 16, 2010 22:45
Version trunk
OS All
CC @tkremenek

Extended Description

The analyser produces this:

  d = [NSDataClass dataWithBytesNoCopy: b length: l];

It appears to be treating this method as returning an owning reference because it contains the word copy. It should be checking if the method starts with copy (e.g. -copy, -copyWithZone:).

tkremenek commented 13 years ago

Fixed here:

http://llvm.org/viewvc/llvm-project?view=rev&revision=122036

tkremenek commented 13 years ago

cloned to rdar://problem/8722072

tkremenek commented 13 years ago

Note, you can use the attribute 'ns_returns_not_retained' to override the behavior.

This attribute is not present in the headers for any of the methods that contain this convention. There are, in fact, no cases in Apple's Foundation or AppKit frameworks where a method contains the word copy anywhere other than at the start, yet returns an owned reference (many return BOOL, or some other non-object type, some return a non-owned reference).

If the conventions are meant to be correct, then I suggest that you remind the Cocoa team that they have been consistently violating them for approximately 20 years (some of the methods of this form are present in the OpenStep specification, while some were introduced as recently as OS X 10.6).

Surely correcting the conventions to reflect how they have been used - and the expectations of developers, formed over many years - is a better solution than creating lots of spurious warnings. It's all very well to say that the conventions have been vetted, but they have clearly not been observed. A simple grep of the headers will show that this rule is wrong.

After more conversation, the Cocoa guys now agree with you. The docs will be updated, and so will the analyzer.

The new rule is that any method that starts with 'copy' or 'mutableCopy' follows the copy rule. Anything else that contains 'copy' will need to use the ns_returns_retained attribute.

davidchisnall commented 13 years ago

Note, you can use the attribute 'ns_returns_not_retained' to override the behavior.

This attribute is not present in the headers for any of the methods that contain this convention. There are, in fact, no cases in Apple's Foundation or AppKit frameworks where a method contains the word copy anywhere other than at the start, yet returns an owned reference (many return BOOL, or some other non-object type, some return a non-owned reference).

If the conventions are meant to be correct, then I suggest that you remind the Cocoa team that they have been consistently violating them for approximately 20 years (some of the methods of this form are present in the OpenStep specification, while some were introduced as recently as OS X 10.6).

Surely correcting the conventions to reflect how they have been used - and the expectations of developers, formed over many years - is a better solution than creating lots of spurious warnings. It's all very well to say that the conventions have been vetted, but they have clearly not been observed. A simple grep of the headers will show that this rule is wrong.

tkremenek commented 13 years ago

Note, you can use the attribute 'ns_returns_not_retained' to override the behavior.

tkremenek commented 13 years ago

Yes, the docs are correct. This convention has been vetted numerous times with the Cocoa guys.

lattner commented 13 years ago

I don't think if it matters if practice differs from the documentation, the docs still win. Should this be closed?

davidchisnall commented 14 years ago

Other examples of methods in Cocoa that contain the word 'copy' but do not return an owned instance:

From Foundation:

[NSString -initWithCharactersNoCopy:length:freeWhenDone:] [NSString -initWithBytesNoCopy:length:encoding:freeWhenDone:]

[NSData +dataWithBytesNoCopy:length:] [NSData +dataWithBytesNoCopy:length:freeWhenDone:] [NSData -initWithBytesNoCopy:length:] [NSData -initWithBytesNoCopy:length:freeWhenDone:]

[NSFileManager -fileManager:shouldCopyItemAtPath:toPath:] [NSFileManager -fileManager:shouldCopyItemAtURL:toURL:]

From AppKit:

[NSCursor.h: +dragCopyCursor]

[NSImageView -allowsCutCopyPaste] [NSImageView -setAllowsCutCopyPaste:] [NSPrintOperation -isCopyingOperation]

In contrast, there are only four that return an owned instance:

These are defined in the NSCopying and NSMutableCopying protocols, respectively, and, as I recall, the OpenStep spec contains a note that they have special ownership rules.

I suspect that the Cocoa guidelines were written by someone who read the NeXT guidelines without fully understanding them. This rule is certainly not followed by the Cocoa team - two of the methods that I listed were added with 10.6.

davidchisnall commented 14 years ago

In that case, the guidelines are either incorrect or are case sensitive. The Cocoa APIs contain several examples of methods that have Copy in their names. Lots of things that take pointers as arguments have either Copy or NoCopy in the argument name.

The one shown in this bug report is a good example - dataWithBytesNoCopy: is a Cocoa method which returns an autoreleased instance.

tkremenek commented 14 years ago

Confirmed. From the Cocoa naming conventions:

"You take ownership of an object if you create it using a method whose name begins with “alloc” or “new” or contains “copy” (for example,alloc, newObject, or mutableCopy), or if you send it a retain message."

Official guidelines here:

http://developer.apple.com/library/mac/#documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html%23//apple_ref/doc/uid/20000994-BAJHFBGH

tkremenek commented 14 years ago

It appears to be treating this method as returning an owning reference because it contains the word copy. It should be checking if the method starts with copy (e.g. -copy, -copyWithZone:).

I think the Cocoa team actually disagrees with this last sentence. "copy" just needs to appear in the method name. I'll ask them for confirmation.

davidchisnall commented 14 years ago

assigned to @tkremenek