search

lloydsargent / BlackRaccoon

IOS FTP Client Code
102 stars 37 forks source link

General improvements #18

Closed albertodebortoli closed 11 years ago

albertodebortoli commented 11 years ago

I hope you appreciate these improvements:

lloydsargent commented 11 years ago

This is the last time I will mention it, but please do not make commits that are changes in spacing. This is standard for open source projects. I don't want a commit war because someone does or doesn't like how things are spaced, tabbed or otherwise formatted.

I'm not trying to be a dick, I just want to avoid the eventual outcome which will be : "how come you allow THEM to change the format spacing and not me?". This is the last and final time.

I appreciate the work y'all have done. Seriously.

albertodebortoli commented 11 years ago

I see @lloydsargent, changing spaces can bother. Just to be clear, I made all of the changes without taking into account a possible future pull request. But since the changes evolved and looked really nice IMHO... I went for a pull request. So... that's it. Thanks for merging it.

lloydsargent commented 11 years ago

Just for the record, I'm keeping it as is and putting in my changes that I haven't committed (these use AppleDoc for documentation - it's a decent tool very similar to Doxygen).

I've seen format wars... it is not pretty.

Thanks for spending the time cleaning this up. Truly, I do appreciate the work y'all have put into it.

On Jun 22, 2013, at 18:17 , Alberto De Bortoli notifications@github.com wrote:

I see @lloydsargent, changing spaces can bother. Just to be clear, I made all of the changes without taking into account a possible future pull request. But since the changes evolved and looked really nice IMHO... I went for a pull request. So... that's it. Thanks for merging it.

— Reply to this email directly or view it on GitHub.

lloydsargent commented 11 years ago

One thing I'm puzzled is why you put the default in switch statements when it does nothing. That's the normal case when something is not found in a switch statement: to do nothing.

On Jun 22, 2013, at 18:17 , Alberto De Bortoli notifications@github.com wrote:

I see @lloydsargent, changing spaces can bother. Just to be clear, I made all of the changes without taking into account a possible future pull request. But since the changes evolved and looked really nice IMHO... I went for a pull request. So... that's it. Thanks for merging it.

— Reply to this email directly or view it on GitHub.

albertodebortoli commented 11 years ago

Because we fixed the warnings this way, first, the compiler complains about it. The default statement should always be present. Good practices.

lloydsargent commented 11 years ago

Interesting. I never had the compiler complain about it. I DID have it complain about the missing enum (which I went ahead and put in despite the fact that it did nothing useful - merely to shut the compiler up).

Are you using the default compile options?

On Jun 22, 2013, at 19:11 , Alberto De Bortoli notifications@github.com wrote:

Because we fixed the warnings this way, first, the compiler complains about it. The default statement should always be present. Good practices.

— Reply to this email directly or view it on GitHub.

albertodebortoli commented 11 years ago

Obviously. Just use the latest stable Xcode version with LLVM 4.2. Btw, I remember that even the oldest GCC compiler I know back to 2004 would complain about missing default in switches.

lloydsargent commented 11 years ago

Hmmm... I just commented out default and there is no error. I've been using GCC since the 90's and don't recall it complaining about missing default (IIRC it complains if I used strict). It's really a moot point since default does nothing it gets optimized out.

I just created a new project and in application:didFinishLaunchingWithOptions added the following code (it doesn't work, I just wanted to see if the compile gave me errors):

switch ([[launchOptions objectForKey: @"blah"] integerValue])
{
    case 1:
        NSLog(@"do nothing");
        break;
    case 2:
        NSLog(@"do nothing");
        break;
    case 3:
        NSLog(@"do nothing");
        break;
}

No errors. It's conceivable that I have a default set (for some weird odd reason, but I don't think so). I'm using XCode 4.6.3. 

Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn)
Target: arm-apple-darwin12.4.0
Thread model: posix

Puzzling.

colinhumber commented 11 years ago

I have the same warnings when default isn't in it. Probably a build setting. It's more just nicer to have no warnings during a compile than anything.

On Jun 23, 2013, at 5:57 PM, lloydsargent notifications@github.com wrote:

Hmmm... I just commented out default and there is no error. I've been using GCC since the 90's and don't recall it complaining about missing default (IIRC it complains if I used strict). It's really a moot point since default does nothing it gets optimized out.

I just created a new project and in application:didFinishLaunchingWithOptions added the following code (it doesn't work, I just wanted to see if the compile gave me errors):

switch ([[launchOptions objectForKey: @"blah"] integerValue]) { case 1: NSLog(@"do nothing"); break; case 2: NSLog(@"do nothing"); break; case 3: NSLog(@"do nothing"); break; }

No errors. It's conceivable that I have a default set (for some weird odd reason, but I don't think so). I'm using XCode 4.6.3.

Apple LLVM version 4.2 (clang-425.0.28) (based on LLVM 3.2svn) Target: arm-apple-darwin12.4.0 Thread model: posix

Puzzling.

— Reply to this email directly or view it on GitHubhttps://github.com/lloydsargent/BlackRaccoon/pull/18#issuecomment-19884163 .

lloydsargent commented 11 years ago

That is what I'm saying is puzzling. I created a brand new project and didn't change any of the build settings. As far as I'm aware the build settings are not persistent between projects.

Are y'all creating custom projects? Or are you using the defaults? Personally, I would like to get this resolved so that another project that I have lined up doesn't create the same havoc.

colinhumber commented 11 years ago

In this case, the warning was because there was a missing case in the switch statement for NSStreamEventNone. If the Check Switch Statements warning is enabled, and the switch statement is based off an enum, in this case NSStreamEvent, you have to provide cases for all enum values. By providing a default case, of a case explicitly for NSStreamEventNone, the warning goes away. The previous code didn't have either, hence the warning.

The test project you were using had a switch based on an arbitrary value. Change it to use an enumerated value and the warning will appear as that warning is enabled by default using the standard Xcode project templates.

lloydsargent commented 11 years ago

Okay, I see.

In my last commit I went ahead and added the NSStreamEventNone as well as kept the default (both get optimized out since they do nothing). I don't normally like putting empty statements like this in code because it too frequently looks like someone "forgot" to add code.