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

Stage 8 of clang compiler warning patches. #115

Merged
merged 4 commits into from
Nov 5, 2015

Conversation

ethoms
Copy link
Contributor

@ethoms ethoms commented Nov 3, 2015

Stage 8: CAUTION - needs testing

Fixes ambiguous boolean logic caused by lack of brackets between AND and OR expressions. Since it's so ambiguous, I had to guess what was the correct logic. Without knowing the user-agent strings involved, this was my best estimation. Note that Ludovic reckons it looks OK in an email conversation. Ideally it needs testing.

)
)
)
&& [[cc userAgent] rangeOfString: @"AddressBook"].location != NSNotFound
Copy link
Contributor

Choose a reason for hiding this comment

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

The address book app on OSX 10.6 uses a localized user-agent string so we can't expect to locate AddressBook. Even the English localization will use Address%20Book as shown in the comments above the method.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 5, 2015

OK, noted. I'll look at those comments. I'd still like to clear the compiler warnings. Even if that just means adding brackets. Does the current code work properly? If so, I'd need to work out how the compiler interprets the logic and add brackets accordingly. I remember in maths the '+' is compounded with priority. E.g. "1 + 2 x 3" is corrected to "(1 + 2) x 3", as opposed to "1 + (2 x 3)". So I need to find the same rules for boolean logic, it's been a while since I learnt this at university.

@ethoms
Copy link
Contributor Author

ethoms commented Nov 5, 2015

I only just noticed the comments on the user agent strings. Silly me, appologies.

Good, so now I can write the logic up better. Those kind of comments are worth gold!

@cgx
Copy link
Contributor

cgx commented Nov 5, 2015

The original condition is working properly. The boolean computation was:

A && B || ( A || C ) && D

which is really:

( A && B ) || ( ( A || C ) && D )

and could be simplified to:

( A && B ) || ( A && D ) || ( C && D )

@extrafu
Copy link
Contributor

extrafu commented Nov 5, 2015

The simplified form is the way to go!

@ethoms
Copy link
Contributor Author

ethoms commented Nov 5, 2015

OK, I have something a bit different to try out. It should be easier to read code, less lines of code and easier to extend in future. But I'm not sure if the logic is 100% correct. I'll edit this PR, and you tell me if it looks OK.

@cgx
Copy link
Contributor

cgx commented Nov 5, 2015

Please just use my simplified form. Your latest commit still assume that Address%20Book is not localized for OS X 10.6 which is wrong.

Thanks for your hard work!

@ethoms
Copy link
Contributor Author

ethoms commented Nov 5, 2015

Sorry, I forgot about the localized string. I've corrected as you suggested:

( A && B ) || ( A && D ) || ( C && D )

ethoms added a commit to ethoms/sogo that referenced this pull request Nov 5, 2015
cgx added a commit that referenced this pull request Nov 5, 2015
Stage 8 of clang compiler warning patches.
@cgx cgx merged commit f3f7ca8 into Alinto:master Nov 5, 2015
@ethoms ethoms deleted the clang-warnings-stage8 branch November 27, 2015 04:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants