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

Fix -Wobjc-signed-char-bool-implicit-int-conversion issue #586

Merged
merged 1 commit into from Apr 4, 2022
Merged

Fix -Wobjc-signed-char-bool-implicit-int-conversion issue #586

merged 1 commit into from Apr 4, 2022

Conversation

matrush
Copy link
Collaborator

@matrush matrush commented Mar 8, 2022

It's an annoying warning and caused my local build to fail when treating warning as error. Let's just fix it.
Error:

stderr: FLEX/src/Classes/Core/Controllers/FLEXTableViewController.m:69:46: error: implicit conversion from integral type 'int' to 'BOOL' [-Werror,-Wobjc-signed-char-bool-implicit-int-conversion]
[CONTEXT]         _manuallyDeactivateSearchOnDisappear = ({
[CONTEXT]                                              ^

@NSExceptional
Copy link
Collaborator

I'm confused, what exactly is the warning complaining about? We're assigning a BOOL a Boolean expression. Can we remove the { curly braces maybe?

I really don't like the empty if statement in the availability check

@matrush
Copy link
Collaborator Author

matrush commented Mar 24, 2022

@NSExceptional I don't really understand the warning either.
availability checks doesn't support ! so another way is to flip the default value and only sets it on iOS 11 and above?

@NSExceptional
Copy link
Collaborator

What happens if you remove the curly braces from inside the parenthesis instead?

…eViewController

It's an annoying warning and caused my local build to fail when treating warning as error. Let's just fix it.
@matrush
Copy link
Collaborator Author

matrush commented Mar 24, 2022

What happens if you remove the curly braces from inside the parenthesis instead?

We should in favor of using @available checks over NSProcessInfo. Technical wise it's definitely working :)

@matrush
Copy link
Collaborator Author

matrush commented Mar 31, 2022

@NSExceptional Good to merge now?

@NSExceptional
Copy link
Collaborator

NSExceptional commented Mar 31, 2022

There's nothing wrong with using NSProcessorInfo; the benefit of @available is that it aids the compiler in removing unnecessary warnings. We shouldn't go out of our way to use it where it makes code less readable

Anyway, I am not able to reproduce the warning at all. What happens if you remove the curly braces from inside the parenthesis instead of all this? I.E.:

_manuallyDeactivateSearchOnDisappear = (
    NSProcessInfo.processInfo.operatingSystemVersion.majorVersion < 11
);

@NSExceptional
Copy link
Collaborator

NSExceptional commented Mar 31, 2022

Also, it seems like you get a lot of warnings I don't get; what are you using to build FLEX? I would like to try it on my machine so I can catch these in advance 🤓

@matrush
Copy link
Collaborator Author

matrush commented Mar 31, 2022

@NSExceptional I think there are many strict flags enabled internally, I think if you use -Wall and -Werror it should report more warnings and they will block building.

For this specific error, you can see it's introduced by -Werror,-Wobjc-signed-char-bool-implicit-int-conversion from my initial comment #586 (comment).

@matrush
Copy link
Collaborator Author

matrush commented Mar 31, 2022

There's nothing wrong with using NSProcessorInfo; the benefit of @available is that it aids the compiler in removing unnecessary warnings. We shouldn't go out of our way to use it where it makes code less readable

Anyway, I am not able to reproduce the warning at all. What happens if you remove the curly braces from inside the parenthesis instead of all this? I.E.:

_manuallyDeactivateSearchOnDisappear = (
    NSProcessInfo.processInfo.operatingSystemVersion.majorVersion < 11
);

It's not ideal to check majorVersion in the code directly. I'm not saying it's technically wrong, but Apple creates @available check for a reason. We should in favor of using @available all times when possible (Other than logging maybe). This way it not only removes dead code on compiling time, but also let us clean up these version checks + deadcode easier in a future min supported version bump.

@matrush matrush merged commit f3a2854 into FLEXTool:master Apr 4, 2022
@matrush
Copy link
Collaborator Author

matrush commented Apr 4, 2022

Merging this for now as it's just about style. We will remove those code when we deprecate iOS 11 and below.

@matrush matrush deleted the fixBoolean branch April 4, 2022 18:46
@tinder-tannerbennett
Copy link
Contributor

tinder-tannerbennett commented Apr 4, 2022

We won't be dropping iOS 11 until the tooling does, which will be a long time; Xcode stills supports iOS 9.

Bleh, I don't really like this change, and I'm getting tired of not being able to see the same warnings as you 😕 Can you help me reproduce it? I am using Xcode 13.1 and I added the warning flags you mentioned and I was not seeing this warning. Can you tell me your entire build stack? Are you using Buck? Which version of Xcode / macOS are you on? Etc. I want to be able to reproduce the warning myself so I can get ahead of these things in the future.

And as I said, @availability is specifically for guarding against deprecation warnings. We are not avoiding a deprecation warning here so there's no need to change perfectly fine code to use it. In fact I even prefer not using it in order to avoid code that is harder to read, like what was just merged. It's only marginally less clear, but still, it's the principle of it for me 🥴

@tinder-tannerbennett
Copy link
Contributor

Sorry if I sound angry, I'm not upset with you, just annoyed with Xcode that I can't see the same warnings as you 😫 Like why is it so inconsistent? lol

@matrush
Copy link
Collaborator Author

matrush commented Apr 4, 2022

@tinder-tannerbennett The error is returned on our server which has many settings so this might not be easily reproducible. However I'm just trying to be good citizen to fix potential error-prone syntax. The flags are the best thing I can provided to you here.

I don't really think @availability really make things less cleaner/readable. I think both cases are easy to understand, if you really feel strongly, I can submit a new patch to use what you recommended.

@matrush
Copy link
Collaborator Author

matrush commented Apr 4, 2022

I just pushed a change to revert to the syntax you preferred.

@NSExceptional
Copy link
Collaborator

I don't really think @Availability really make things less cleaner/readable. I think both cases are easy to understand, if you really feel strongly, I can submit a new patch to use what you recommended.

I'm being nitpicky, you didn't have to revert it 😅 My real concern is that I'm not able to reproduce the warning at all, so from my point of view it's a neutral or net-negative change. I believe you're seeing it, I just wish I could reproduce it.

If you could pry and get me any further details about your build configuration, I would really appreciate it 🙏🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants