Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stage1 of clang compiler warning patches. #108

Merged
merged 9 commits into from
Nov 4, 2015

Conversation

ethoms
Copy link
Contributor

@ethoms ethoms commented Nov 3, 2015

Stage 1:

Fairly rudimentary fixes. Mostly fixing data type and casting inconsistencies.

Fixes for:
(i) Data type casting
(ii) Data type initialization
(iii) String formatting (data types)
(iv) Missing curly brackets in ambiguous if statements / for loops.

@ethoms ethoms changed the title Clang warnings stage1 Stage1 of clang compiler warning patches. Nov 3, 2015
@@ -94,7 +94,7 @@ - (id) initWithName: (NSString *) newName
{
if (![newDisplayName length])
newDisplayName = newName;
ASSIGN (displayName, newDisplayName);
ASSIGN (displayName, [newDisplayName mutableCopy]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? You'll leak here.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

The reason I used mutableCopy is to (I thought) safely get a mutable instance. Since the compiler warns that it is the wrong data type being received. E.g. an NSMutableString is expected, but a NSString is given. Initially I just did a simple cast (NSMutableString *), but after reading that it's better to cast properly using mutableCopy I changed the patches.

Is it safe to use a simple cast (NSMutableString *) etc.? Or do have any better ideas to get the data types corrected so to avoid compiler warnings?

I'm not sure what you mean by leaking? I guess it should be passed by reference, not by copy?

@extrafu
Copy link
Contributor

extrafu commented Nov 3, 2015

mutableCopy creates a new instance with a retain count of 1, so it's not autoreleased and thus, leaks the newly created instance.

Just cast it, it'll be good!

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

OK, noted. So I'll just change these and add another commit to this PR?

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

Actually, I'll just use the github editor. I'll post back after build testing on FreeBSD and RHEL 5.11.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

OK, all done. Converted all uses of mutableCopy to a cast in this pull request.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

NOTE: on line 168 of UI/Scheduler/UIxCalDayView.m there is a use of mutableCopy. It's not one of my changes, but maybe you want to check it out.

@extrafu
Copy link
Contributor

extrafu commented Nov 3, 2015

Yep, but notice it's returned as autoreleased at the end of the method.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 3, 2015

Ah, OK, I didn't read on.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 4, 2015

I've successfully build tested on both FreeBSD 10.1 and RHEL 5.11 after correcting the mutableCopy issue.

Is there any other issue with this PR?

@extrafu
Copy link
Contributor

extrafu commented Nov 4, 2015

I don't think so but I'll wait until our nightly builds + integration tests pass in a couple of hours before merging new stuff.

Thanks!

extrafu added a commit that referenced this pull request Nov 4, 2015
Stage1 of clang compiler warning patches.
@extrafu extrafu merged commit cc1555a into Alinto:master Nov 4, 2015
@extrafu
Copy link
Contributor

extrafu commented Nov 4, 2015

Can you please adapt this PR so we can merge it also in the v2 branch? I guess it'll be mostly code remove.

Open the new PR just for the v2 branch - since the patch was merged in master.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 5, 2015

No problem, I'll make a new PR to the v2 branch as soon as I've done looking into the other issues.

@ethoms ethoms deleted the clang-warnings-stage1 branch November 27, 2015 04:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants