Permalink
Browse files

performed a code review, specifically:

- changed some #include to #import
- fixed some NULL -> nil Cocoa coding conventions
- added new compiler warnings and fixed some warnings they generated
- check for nil from NSTemporaryDirectory
- added missing files to unit test and test app targets
- added xcconfig files for unit test target
- added @private to some ivars
- changes some variables from signed to unsigned as appropriate
- changed from base 2 to base 10 measurements of file size, consistent with both the actual meaning of metric prefixes and Apple's new policy as of 10.6
- reduced some unneeded copy-paste of code
- fixed failure to check for null from malloc and unneeded check against null before calling free
- OSErr was incorrectly used instead of OSStatus
- added some consts & statics to global strings
- fixed some issues discovered by static analysis
- fixed some 64bit issues, mostly related to casting and the use of slightly incorrect types/sizes
- some dealloc methods were using accessors, changed to access ivars directly, as per Apple guidelines
- removed old NS_DURING, NS_HANDLER, NS_ENDHANDLER macros
- fixed a bug where immutable data was being mutated
- removed all instance of "== YES" as they are dangerous
- removed some redundant nil checks
- fixed some leaks
- conditionally replaced deprecated method usage
- cleanup CF/NSMakeCollectable usage
- fixed bug in GC where memory could be collected too early due to lack of strong references when using UTF8String
- prevent passing null to CFRelease
  • Loading branch information...
1 parent bd80144 commit d7774c0dc039e30f32f2e66a4c9e5affb2f67204 Sean McBride committed Nov 1, 2009
@@ -14,6 +14,7 @@ GCC_DEBUGGING_SYMBOLS = full
GCC_PRECOMPILE_PREFIX_HEADER = YES
GCC_PREFIX_HEADER = $(SDKROOT)/System/Library/Frameworks/Cocoa.framework/Headers/Cocoa.h
GCC_FAST_OBJC_DISPATCH = YES
+GCC_ENABLE_PASCAL_STRINGS = NO
ARCHS = ppc i386 x86_64
// Enable warnings
@@ -36,4 +37,15 @@ GCC_WARN_UNKNOWN_PRAGMAS = YES
GCC_WARN_UNUSED_VARIABLE = YES
GCC_WARN_ABOUT_DEPRECATED_FUNCTIONS = YES
GCC_WARN_ABOUT_INVALID_OFFSETOF_MACRO = YES
-WARNING_CFLAGS = -Wall
+GCC_WARN_ABOUT_MISSING_PROTOTYPES = YES
+GCC_WARN_HIDDEN_VIRTUAL_FUNCTIONS = YES
+GCC_WARN_ABOUT_INVALID_OFFSETOF_MACRO = YES
+GCC_WARN_UNUSED_FUNCTION = YES
+GCC_WARN_UNUSED_LABEL = YES
+GCC_WARN_UNUSED_VALUE = YES
+GCC_WARN_UNUSED_PARAMETER = YES
+GCC_WARN_ABOUT_MISSING_FIELD_INITIALIZERS = YES
+WARNING_CFLAGS = -Wall -Wundef -Wendif-labels -Wpointer-arith -Wcast-align -Wwrite-strings -Wmissing-noreturn -Wmissing-format-attribute -Wpacked -Wredundant-decls -Winline -Wdisabled-optimization -Wformat=2 -Wlarger-than-32768 -Winvalid-pch
+
+// TODO:
+// GCC_WARN_UNDECLARED_SELECTOR = YES only 7 warnings
@@ -3,5 +3,8 @@
GCC_OPTIMIZATION_LEVEL = 0
DEBUG_INFORMATION_FORMAT = dwarf
GCC_GENERATE_DEBUGGING_SYMBOLS = YES
-SPARKLE_EXTRA_DEBUG = -DDEBUG -fstack-protector -D_FORTIFY_SOURCE=2
-OTHER_CFLAGS = $(SPARKLE_EXTRA_DEBUG)
+SPARKLE_EXTRA_DEBUG_10_5_ONLY = -fstack-protector -D_FORTIFY_SOURCE=2
+SPARKLE_EXTRA_DEBUG = -DDEBUG
+OTHER_CFLAGS = $(SPARKLE_EXTRA_DEBUG)
+
+// Add $(SPARKLE_EXTRA_DEBUG_10_5_ONLY) to SPARKLE_EXTRA_DEBUG if your deployment is 10.5 or greater.
@@ -0,0 +1,9 @@
+// Unit Test only
+
+INFOPLIST_FILE = Tests/Sparkle Unit Tests-Info.plist
+OTHER_LDFLAGS = -framework Cocoa -framework SenTestingKit
+PRODUCT_NAME = Sparkle Unit Tests
+WRAPPER_EXTENSION = octest
+FRAMEWORK_SEARCH_PATHS = $(DEVELOPER_LIBRARY_DIR)/Frameworks
+GCC_PREFIX_HEADER = $(SYSTEM_LIBRARY_DIR)/Frameworks/Cocoa.framework/Headers/Cocoa.h
+GCC_PRECOMPILE_PREFIX_HEADER = YES
@@ -0,0 +1,3 @@
+#include "ConfigCommon.xcconfig"
+#include "ConfigCommonDebug.xcconfig"
+#include "ConfigUnitTest.xcconfig"
@@ -0,0 +1,3 @@
+#include "ConfigCommon.xcconfig"
+#include "ConfigCommonRelease.xcconfig"
+#include "ConfigUnitTest.xcconfig"
@@ -0,0 +1,3 @@
+#include "ConfigUnitTestRelease.xcconfig"
+
+GCC_ENABLE_OBJC_GC = required
View
@@ -11,6 +11,7 @@
@interface NTSynchronousTask : NSObject
{
+@private
NSTask *mv_task;
NSPipe *mv_outputPipe;
NSPipe *mv_inputPipe;
View
@@ -56,10 +56,10 @@ - (void)dealloc
{
[[NSNotificationCenter defaultCenter] removeObserver:self];
- [self setTask:nil];
- [self setOutputPipe:nil];
- [self setInputPipe:nil];
- [self setOutput:nil];
+ [mv_task release];
+ [mv_outputPipe release];
+ [mv_inputPipe release];
+ [mv_output release];
[super dealloc];
}
@@ -70,7 +70,7 @@ + (NSData*)task:(NSString*)toolPath directory:(NSString*)currentDirectory withAr
NSAutoreleasePool *pool = [[NSAutoreleasePool alloc] init];
NSData* result=nil;
- NS_DURING
+ @try
{
NTSynchronousTask* task = [[NTSynchronousTask alloc] init];
@@ -81,8 +81,7 @@ + (NSData*)task:(NSString*)toolPath directory:(NSString*)currentDirectory withAr
[task release];
}
- NS_HANDLER;
- NS_ENDHANDLER;
+ @catch (NSException *localException) { }
[pool drain];
@@ -92,10 +91,6 @@ + (NSData*)task:(NSString*)toolPath directory:(NSString*)currentDirectory withAr
return result;
}
-@end
-
-@implementation NTSynchronousTask (Private)
-
- (void)run:(NSString*)toolPath directory:(NSString*)currentDirectory withArgs:(NSArray*)args input:(NSData*)input;
{
BOOL success = NO;
@@ -118,12 +113,12 @@ - (void)run:(NSString*)toolPath directory:(NSString*)currentDirectory withArgs:(
[[[self outputPipe] fileHandleForReading] readToEndOfFileInBackgroundAndNotifyForModes:[NSArray arrayWithObjects:NSDefaultRunLoopMode, NSModalPanelRunLoopMode, NSEventTrackingRunLoopMode, nil]];
- NS_DURING
+ @try
+ {
[[self task] launch];
success = YES;
- NS_HANDLER
- ;
- NS_ENDHANDLER
+ }
+ @catch (NSException *localException) { }
if (success)
{
View
@@ -10,7 +10,9 @@
#define SUAPPCAST_H
@class SUAppcastItem;
-@interface SUAppcast : NSObject {
+@interface SUAppcast : NSObject
+{
+@private
NSArray *items;
NSString *userAgentString;
id delegate;
View
@@ -40,8 +40,12 @@ - (void)fetchAppcastFromURL:(NSURL *)url
- (void)download:(NSURLDownload *)download decideDestinationWithSuggestedFilename:(NSString *)filename
{
- NSString *destinationFilename = [NSTemporaryDirectory() stringByAppendingPathComponent:filename];
- [download setDestination:destinationFilename allowOverwrite:NO];
+ NSString* destinationFilename = NSTemporaryDirectory();
+ if (destinationFilename)
+ {
+ destinationFilename = [destinationFilename stringByAppendingPathComponent:filename];
+ [download setDestination:destinationFilename allowOverwrite:NO];
+ }
}
- (void)download:(NSURLDownload *)download didCreateDestination:(NSString *)path
@@ -52,7 +56,8 @@ - (void)download:(NSURLDownload *)download didCreateDestination:(NSString *)path
- (void)downloadDidFinish:(NSURLDownload *)download
{
- CFRelease(download);
+ if (download)
+ CFRelease(download);
NSError *error = nil;
NSXMLDocument *document = [[NSXMLDocument alloc] initWithContentsOfURL:[NSURL fileURLWithPath:downloadFilename] options:0 error:&error];
@@ -62,7 +67,7 @@ - (void)downloadDidFinish:(NSURLDownload *)download
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
[[NSFileManager defaultManager] removeFileAtPath:downloadFilename handler:nil];
#else
- [[NSFileManager defaultManager] removeItemAtPath:downloadFilename error:NULL];
+ [[NSFileManager defaultManager] removeItemAtPath:downloadFilename error:nil];
#endif
[downloadFilename release];
downloadFilename = nil;
@@ -179,13 +184,14 @@ - (void)downloadDidFinish:(NSURLDownload *)download
- (void)download:(NSURLDownload *)download didFailWithError:(NSError *)error
{
- CFRelease(download);
+ if (download)
+ CFRelease(download);
if (downloadFilename)
{
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
[[NSFileManager defaultManager] removeFileAtPath:downloadFilename handler:nil];
#else
- [[NSFileManager defaultManager] removeItemAtPath:downloadFilename error:NULL];
+ [[NSFileManager defaultManager] removeItemAtPath:downloadFilename error:nil];
#endif
}
[downloadFilename release];
@@ -219,7 +225,7 @@ - (NSXMLNode *)bestNodeInNodes:(NSArray *)nodes
NSXMLElement *node;
NSMutableArray *languages = [NSMutableArray array];
NSString *lang;
- NSInteger i;
+ NSUInteger i;
while ((node = [nodeEnum nextObject]))
{
lang = [[node attributeForName:@"xml:lang"] stringValue];
View
@@ -9,7 +9,9 @@
#ifndef SUAPPCASTITEM_H
#define SUAPPCASTITEM_H
-@interface SUAppcastItem : NSObject {
+@interface SUAppcastItem : NSObject
+{
+@private
NSString *title;
NSDate *date;
NSString *itemDescription;
View
@@ -185,14 +185,14 @@ - (void)setMinimumSystemVersion:(NSString *)systemVersionString
- (void)dealloc
{
- [self setTitle:nil];
- [self setDate:nil];
- [self setItemDescription:nil];
- [self setReleaseNotesURL:nil];
- [self setDSASignature:nil];
- [self setFileURL:nil];
- [self setVersionString:nil];
- [self setDisplayVersionString:nil];
+ [title release];
+ [date release];
+ [itemDescription release];
+ [releaseNotesURL release];
+ [DSASignature release];
+ [fileURL release];
+ [versionString release];
+ [displayVersionString release];
[propertiesDictionary release];
[super dealloc];
}
@@ -13,7 +13,9 @@
#import "SUBasicUpdateDriver.h"
@class SUAutomaticUpdateAlert;
-@interface SUAutomaticUpdateDriver : SUBasicUpdateDriver {
+@interface SUAutomaticUpdateDriver : SUBasicUpdateDriver
+{
+@private
BOOL postponingInstallation, showErrors;
SUAutomaticUpdateAlert *alert;
}
View
@@ -94,7 +94,7 @@ - (void)appcastDidFinishLoading:(SUAppcast *)ac
}
updateItem = [item retain];
- CFRelease(ac); // Remember that we're explicitly managing the memory of the appcast.
+ if (ac) { CFRelease(ac); } // Remember that we're explicitly managing the memory of the appcast.
if (updateItem == nil) { [self didNotFindUpdate]; return; }
if ([self itemContainsValidUpdate:updateItem])
@@ -105,7 +105,7 @@ - (void)appcastDidFinishLoading:(SUAppcast *)ac
- (void)appcast:(SUAppcast *)ac failedToLoadWithError:(NSError *)error
{
- CFRelease(ac); // Remember that we're explicitly managing the memory of the appcast.
+ if (ac) { CFRelease(ac); } // Remember that we're explicitly managing the memory of the appcast.
[self abortUpdateWithError:error];
}
@@ -139,27 +139,31 @@ - (void)download:(NSURLDownload *)d decideDestinationWithSuggestedFilename:(NSSt
// We create a temporary directory in /tmp and stick the file there.
// Not using a GUID here because hdiutil (for DMGs) for some reason chokes on GUIDs. Too long? I really have no idea.
NSString *prefix = [NSString stringWithFormat:@"%@ %@ Update", [host name], [host version]];
- NSString *tempDir = [NSTemporaryDirectory() stringByAppendingPathComponent:prefix];
- int cnt=1;
- while ([[NSFileManager defaultManager] fileExistsAtPath:tempDir] && cnt <= 999)
+ NSString *tempDir = NSTemporaryDirectory();
+ if (tempDir)
{
- tempDir = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSString stringWithFormat:@"%@ %d", prefix, cnt++]];
- }
+ tempDir = [tempDir stringByAppendingPathComponent:prefix];
+ unsigned int cnt=1;
+ while ([[NSFileManager defaultManager] fileExistsAtPath:tempDir] && cnt <= 999)
+ {
+ tempDir = [NSTemporaryDirectory() stringByAppendingPathComponent:[NSString stringWithFormat:@"%@ %u", prefix, cnt++]];
+ }
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
- BOOL success = [[NSFileManager defaultManager] createDirectoryAtPath:tempDir attributes:nil];
+ BOOL success = [[NSFileManager defaultManager] createDirectoryAtPath:tempDir attributes:nil];
#else
- BOOL success = [[NSFileManager defaultManager] createDirectoryAtPath:tempDir withIntermediateDirectories:YES attributes:nil error:NULL];
+ BOOL success = [[NSFileManager defaultManager] createDirectoryAtPath:tempDir withIntermediateDirectories:YES attributes:nil error:nil];
#endif
- if (!success)
- {
- // Okay, something's really broken with /tmp
- [download cancel];
- [self abortUpdateWithError:[NSError errorWithDomain:SUSparkleErrorDomain code:SUTemporaryDirectoryError userInfo:[NSDictionary dictionaryWithObject:[NSString stringWithFormat:@"Can't make a temporary directory for the update download at %@.",tempDir] forKey:NSLocalizedDescriptionKey]]];
+ if (!success)
+ {
+ // Okay, something's really broken with /tmp
+ [download cancel];
+ [self abortUpdateWithError:[NSError errorWithDomain:SUSparkleErrorDomain code:SUTemporaryDirectoryError userInfo:[NSDictionary dictionaryWithObject:[NSString stringWithFormat:@"Can't make a temporary directory for the update download at %@.",tempDir] forKey:NSLocalizedDescriptionKey]]];
+ }
+
+ downloadPath = [[tempDir stringByAppendingPathComponent:name] retain];
+ [download setDestination:downloadPath allowOverwrite:YES];
}
-
- downloadPath = [[tempDir stringByAppendingPathComponent:name] retain];
- [download setDestination:downloadPath allowOverwrite:YES];
}
- (void)downloadDidFinish:(NSURLDownload *)d
@@ -226,16 +230,17 @@ - (void)installUpdate
{
if ([[updater delegate] respondsToSelector:@selector(updater:willInstallUpdate:)])
[[updater delegate] updater:updater willInstallUpdate:updateItem];
+
// Copy the relauncher into a temporary directory so we can get to it after the new version's installed.
- NSString *relaunchPathToCopy = [[NSBundle bundleForClass:[self class]] pathForResource:@"relaunch" ofType:@""];
+ NSString *relaunchPathToCopy = [[NSBundle bundleForClass:[self class]] pathForResource:@"relaunch" ofType:@""];
NSString *targetPath = [NSTemporaryDirectory() stringByAppendingPathComponent:[relaunchPathToCopy lastPathComponent]];
// Only the paranoid survive: if there's already a stray copy of relaunch there, we would have problems.
NSError *error = nil;
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
[[NSFileManager defaultManager] removeFileAtPath:targetPath handler:nil];
if ([[NSFileManager defaultManager] copyPath:relaunchPathToCopy toPath:targetPath handler:nil])
#else
- [[NSFileManager defaultManager] removeItemAtPath:targetPath error:NULL];
+ [[NSFileManager defaultManager] removeItemAtPath:targetPath error:nil];
if ([[NSFileManager defaultManager] copyItemAtPath:relaunchPathToCopy toPath:targetPath error:&error])
#endif
relaunchPath = [targetPath retain];
@@ -292,7 +297,7 @@ - (void)cleanUp
#if MAC_OS_X_VERSION_MIN_REQUIRED <= MAC_OS_X_VERSION_10_4
[[NSFileManager defaultManager] removeFileAtPath:[downloadPath stringByDeletingLastPathComponent] handler:nil];
#else
- [[NSFileManager defaultManager] removeItemAtPath:[downloadPath stringByDeletingLastPathComponent] error:NULL];
+ [[NSFileManager defaultManager] removeItemAtPath:[downloadPath stringByDeletingLastPathComponent] error:nil];
#endif
}
View
@@ -11,26 +11,26 @@
#define SUCONSTANTS_H
-extern NSString *SUUpdaterWillRestartNotification;
-extern NSString *SUTechnicalErrorInformationKey;
+extern NSString *const SUUpdaterWillRestartNotification;
+extern NSString *const SUTechnicalErrorInformationKey;
-extern NSString *SUFeedURLKey;
-extern NSString *SUHasLaunchedBeforeKey;
-extern NSString *SUShowReleaseNotesKey;
-extern NSString *SUSkippedVersionKey;
-extern NSString *SUScheduledCheckIntervalKey;
-extern NSString *SULastCheckTimeKey;
-extern NSString *SUPublicDSAKeyKey;
-extern NSString *SUPublicDSAKeyFileKey;
-extern NSString *SUAutomaticallyUpdateKey;
-extern NSString *SUAllowsAutomaticUpdatesKey;
-extern NSString *SUEnableAutomaticChecksKey;
-extern NSString *SUEnableAutomaticChecksKeyOld;
-extern NSString *SUEnableSystemProfilingKey;
-extern NSString *SUSendProfileInfoKey;
-extern NSString *SULastProfileSubmitDateKey;
+extern NSString *const SUFeedURLKey;
+extern NSString *const SUHasLaunchedBeforeKey;
+extern NSString *const SUShowReleaseNotesKey;
+extern NSString *const SUSkippedVersionKey;
+extern NSString *const SUScheduledCheckIntervalKey;
+extern NSString *const SULastCheckTimeKey;
+extern NSString *const SUPublicDSAKeyKey;
+extern NSString *const SUPublicDSAKeyFileKey;
+extern NSString *const SUAutomaticallyUpdateKey;
+extern NSString *const SUAllowsAutomaticUpdatesKey;
+extern NSString *const SUEnableAutomaticChecksKey;
+extern NSString *const SUEnableAutomaticChecksKeyOld;
+extern NSString *const SUEnableSystemProfilingKey;
+extern NSString *const SUSendProfileInfoKey;
+extern NSString *const SULastProfileSubmitDateKey;
-extern NSString *SUSparkleErrorDomain;
+extern NSString *const SUSparkleErrorDomain;
// Appcast phase errors.
extern OSStatus SUAppcastParseError;
extern OSStatus SUNoUpdateError;
Oops, something went wrong.

0 comments on commit d7774c0

Please sign in to comment.