Closed
Bug 330904
Opened 19 years ago
Closed 18 years ago
Fix more compiler warnings
Categories
(Camino Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino1.6
People
(Reporter: bugzilla-graveyard, Assigned: stuart.morgan+bugzilla)
References
Details
(Keywords: fixed1.8.1.5)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
jaas
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
Patch coming to fix more missing casts and similar warnings.
Reporter | ||
Comment 1•19 years ago
|
||
There are a few remaining warnings in the code that I (mostly) have no idea how to fix or whether they're even important. I'm posting them here: mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:52: warning: '-release' not found in protocol(s) mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:61: warning: '-retain' not found in protocol(s) mozilla/camino/src/browser/RemoteDataProvider.mm:70: warning: '-retain' not found in protocol(s) mozilla/camino/src/browser/RemoteDataProvider.mm:77: warning: '-release' not found in protocol(s) mozilla/camino/src/embedding/CHBrowserListener.mm:352: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873) mozilla/camino/src/embedding/CHBrowserListener.mm:352: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873) mozilla/camino/src/embedding/CHBrowserListener.mm:403: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873) mozilla/camino/src/embedding/CHBrowserListener.mm:403: warning: 'GetMainDevice' is deprecated (declared at /Developer/SDKs/MacOSX10.4u.sdk/System/Library/Frameworks/ApplicationServices.framework/Frameworks/QD.framework/Headers/Quickdraw.h:4873) (These are probably 10.4-only; we should keep it them mind for whatever version drops support for 10.3.) mozilla/camino/src/extensions/ExtendedOutlineView.mm:613: warning: 'NSOutlineView' may not respond to '-_sendDelegateDidClickColumn:' (This may be a 10.2-only workaround issue that we can drop after 1.0.) mozilla/camino/src/browser/KeychainService.mm:225: warning: converting to non-pointer type 'char' from NULL mozilla/camino/src/browser/KeychainService.mm:234: warning: converting to non-pointer type 'char' from NULL mozilla/camino/src/extensions/ExtendedTableView.mm:120: warning: no '-tableView:contextMenuForRow:' method found
Assignee | ||
Comment 3•19 years ago
|
||
Comment on attachment 215473 [details] [diff] [review] fixes warnings and a spelling error Stealing review from Mark in order to say: - (void)setupFontSampleOfType:(NSString*)fontType fromDict:(NSDictionary*)regionDict { NSFont *foundFont = [self getFontOfType:fontType fromDict:regionDict]; - [self setFontSampleOfType:fontType withFont:foundFont andDict:regionDict]; + [self setFontSampleOfType:fontType withFont:foundFont andDict:(NSMutableDictionary*)regionDict]; } - (void)setFontSampleOfType:(NSString *)fontType withFont:(NSFont*)font andDict:(NSMutableDictionary*)regionDict Dear lord no. Never, ever, ever cast a random NSFoo to NSMutableFoo. It's an invitation to pain and suffering later. The cast is highlighting a bad assumption in the code. setFontSampleOfType:withFont:andDict: doesn't appear to ever change the dictionary (which may be the only reason this has never exploded), so the correct fix is to change the argument type.
Attachment #215473 -
Flags: review?(mark) → review-
Reporter | ||
Comment 4•19 years ago
|
||
Attachment #215473 -
Attachment is obsolete: true
Attachment #215522 -
Flags: superreview?(mark)
Attachment #215522 -
Flags: review?(stuart.morgan)
Assignee | ||
Comment 5•19 years ago
|
||
Comment on attachment 215522 [details] [diff] [review] updated per Stuart's comment, fixes last two warnings in my comment 1 Sorry, I missed a few other things because the cast bothered me so much. >- buffer[actualSize] = NULL; >+ buffer[actualSize] = 0; ... >- buffer[actualSize] = NULL; >+ buffer[actualSize] = 0; 0 isn't really any better than NULL. It's a character array, so set it to a character explicitly: buffer[actualSize] = '\0'; >+-(NSMenu*)tableView:(NSTableView *)aTableView contextMenuForRow:(int)rowIndex; ... >+#import "BookmarkViewController.h" ... > if ([delegate respondsToSelector:@selector(tableView:contextMenuForRow:)]) >- return [delegate tableView:self contextMenuForRow:rowIndex]; >+ return [(BookmarkViewController*)delegate tableView:self contextMenuForRow:rowIndex]; This isn't okay. ExtendedTableView is a general-puprpose class, and you are making it directly dependent on a specific other class. This is exactly what protocols are designed to avoid--if you really want to formalize the relationship the right way to do this is to make a protocol with that method and make BVC conform to the protocol. However, since it is already using introspection to verify the presence of the method, and has a fallback return value, I don't think it needs to be changed at all. > - (void) setURLs:(NSArray*)inUrls withTitles:(NSArray*)inTitles > { > // Best format that we know about is Safari's URL + title arrays - build these up >- NSMutableArray* tmpTitleArray = inTitles; >+ NSMutableArray* tmpTitleArray = (NSMutableArray*)inTitles; > if (!inTitles) { > tmpTitleArray = [NSMutableArray array]; > for ( unsigned int i = 0; i < [inUrls count]; ++i ) > [tmpTitleArray addObject:@""]; > } Oops, missed this too. Once again, no dice. This will need to be restructured a tiny bit; the only reason it needs to be mutable is in the case where inTitles is nil, so the logic should be something like: NSArray* tmpTitleArray = inTitles; if (!tmpTitleArray) { //declare and create an NSMutableArray, and fill it with stuff //tmpTitleArray = the new NSMutableArray }
Attachment #215522 -
Flags: review?(stuart.morgan) → review-
Assignee | ||
Comment 6•19 years ago
|
||
By the way, the deprecation warnings are because the entirety of quickdraw is officially deprecated in Tiger. There are presumably a metric ton of deprecation warnings about QD when building the entire project including core.
Assignee | ||
Comment 7•19 years ago
|
||
(In reply to comment #1) > There are a few remaining warnings in the code that I (mostly) have no idea how > to fix or whether they're even important. I'm posting them here: > > mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:52: warning: > '-release' not found in protocol(s) > mozilla/camino/src/embedding/CHDownloadProgressDisplay.mm:61: warning: > '-retain' not found in protocol(s) > > mozilla/camino/src/browser/RemoteDataProvider.mm:70: warning: '-retain' not > found in protocol(s) > mozilla/camino/src/browser/RemoteDataProvider.mm:77: warning: '-release' not > found in protocol(s) In both cases, this is an object that is being accessed entirely in terms of a protocol, but having methods sent to it (retain and release) that are not in fact part of the protocol. You can fix it by adding retain and release to the protocols in question (CHDownloadDisplayFactory and RemoteLoadListener)
Comment 8•19 years ago
|
||
Why do that when you can do this: @protocol CHDownloadDisplayFactory <NSObject> to bring -release and -retain in from NSObject, where they're supposed to come from.
Assignee | ||
Comment 9•19 years ago
|
||
(In reply to comment #8) > Why do that when you can do this: Because I'm dumb, and forgot there's an NSObject protocol? ;)
Reporter | ||
Comment 10•19 years ago
|
||
(In reply to comment #6) > By the way, the deprecation warnings are because the entirety of quickdraw is > officially deprecated in Tiger. There are presumably a metric ton of > deprecation warnings about QD when building the entire project including core. Yep :) Would it be appropriate to replace those calls in Camino with CGMainDisplayID (the Quartz equivalent of QD's GetMainDevice)? I can't seem to find anything on Apple's site about when CGMainDisplayID first appeared, though. cl
Reporter | ||
Comment 11•19 years ago
|
||
Found it: http://developer.apple.com/documentation/GraphicsImaging/Reference/Quartz_Services_Ref/Reference/reference.html cl
Assignee | ||
Comment 12•19 years ago
|
||
(In reply to comment #10) > Would it be appropriate to replace those calls in Camino with CGMainDisplayID > (the Quartz equivalent of QD's GetMainDevice)? I can't seem to find anything on > Apple's site about when CGMainDisplayID first appeared, though. Probably switching to CGMainDisplayID+CGDisplayBounds makes sense, but you'd have to tweak a bit of the rect-extracting code, and you'd have to be very sure that you still ended up with the same idea of what the coordinates were. That would also mean that testing this would require a multi-monitor set-up and some run-time comparison of values to be sure you didn't regress anything, so I would suggest not doing it in this bug.
Updated•19 years ago
|
Attachment #215522 -
Flags: superreview?(mark)
This is the old patches police. What's the status of this bug? Is there a fix for the last patch that will make it worthwhile (i.e., not involving the QD/Quartz stuff)?
QA Contact: general
Target Milestone: --- → Camino1.2
Assignee | ||
Comment 14•18 years ago
|
||
(In reply to comment #13) > Is there a fix for the last patch that will > make it worthwhile (i.e., not involving the QD/Quartz stuff)? Making the changes from comment 5 and comment 8
Assignee | ||
Comment 15•18 years ago
|
||
The last patch is largely obsolete; this covers most of the current warnings.
Assignee: cl-bugs → stuart.morgan
Attachment #215522 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #264540 -
Flags: review?
Attachment #264540 -
Attachment is patch: true
Attachment #264540 -
Attachment mime type: application/octet-stream → text/plain
Comment 16•18 years ago
|
||
Comment on attachment 264540 [details] [diff] [review] updated fixes >+// TODO: This code modifies sub-dictionaries of regionDict, which works only >+// because they happen to have been constructed as mutableDictionaries. This >+// API (and likely others in this class) should be re-worked to either remove or >+// enforce that assumption. >+- (void)setFontSampleOfType:(NSString *)fontType withFont:(NSFont*)font andDict:(NSDictionary*)regionDict Oh my. There has to be a better way!
Assignee | ||
Comment 17•18 years ago
|
||
(In reply to comment #16) > Oh my. There has to be a better way! Yeah, it's pretty skanky, but fixing it is out of the scope of this bug; I just wanted to make sure we don't forget.
Attachment #264540 -
Flags: review? → review+
Assignee | ||
Updated•18 years ago
|
Attachment #264540 -
Flags: superreview?(mikepinkerton)
Comment 19•18 years ago
|
||
Comment on attachment 264540 [details] [diff] [review] updated fixes sr=pink
Attachment #264540 -
Flags: superreview?(mikepinkerton) → superreview+
You need to log in
before you can comment on or make changes to this bug.
Description
•