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 some invalid msg_send! usage #2138

Merged
merged 2 commits into from Jan 10, 2022
Merged

Conversation

madsmtm
Copy link
Member

@madsmtm madsmtm commented Jan 5, 2022

Change a few types for more correct behaviour (e.g. using bool may be undefined behaviour, one should use BOOL).

The only critical thing here is the stuff in unmark_text (introduced in #2119), that part is certainly undefined behaviour, and will probably cause problems somewhere down the line.

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

@madsmtm madsmtm added B - bug Dang, that shouldn't have happened DS - macos DS - ios S - maintenance Repaying technical debt labels Jan 5, 2022
Copy link
Contributor

@ArturKovacs ArturKovacs left a comment

Choose a reason for hiding this comment

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

Thank you for making these improvements.

There is one thing that bugs me. The fact that we have to write

my_objc_bool != NO

I know that it's necessary to compare against NO, but then we have a double negation for every single truthy check.

Here's an alternative:

trait BoolYeah {
    /// This is true when `self` is not `NO`
    /// 
    /// Note that this different from
    /// `self == YES`
    /// because YES is the value 1 but
    /// a BOOL may be any signed 8bit value
    fn yeah(self) -> bool;
    fn no(self) -> bool;
}
impl BoolYeah for objc::runtime::BOOL {
    #[inline]
    fn yeah(self) -> bool {
        self != 0
    }
    #[inline]
    fn no(self) -> bool {
        self == 0
    }
}

So that we can write

if window_has_focus.yeah() {

src/platform_impl/macos/util/mod.rs Outdated Show resolved Hide resolved
src/platform_impl/macos/view.rs Show resolved Hide resolved
@aloucks
Copy link
Contributor

aloucks commented Jan 8, 2022

trait BoolYeah

It would be more clear to have is_true() and is_false() instead of yeah() and no().

@ArturKovacs
Copy link
Contributor

Just to throw another option into the mix, what about truthy() and falsy()? This naming comes from JavaScript.

@maroider
Copy link
Member

maroider commented Jan 9, 2022

Maybe just a conversion, e.g. .to_bool(), would be good enough.
If not, then some variation of truthy and falsy sounds good to me.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 10, 2022

In my fork of objc I've added a Bool newtype, which has as_bool (and is_true and is_false for those that want to use that) to make this exact thing better.

I intend to use that fork in winit at some point, but for this PR I wanted to keep the focus on correctness over niceness.

@ArturKovacs
Copy link
Contributor

Alright, I'm looking forward to moving to your fork of objc then.

@madsmtm
Copy link
Member Author

madsmtm commented Jan 10, 2022

Alright, I'm looking forward to moving to your fork of objc then.

Thanks! Though intending to move to something else in the unknown future shouldn't be a blocker for adding BoolYeah now, just not something I'd want to do in this PR in any case.

Copy link
Contributor

@ArturKovacs ArturKovacs left a comment

Choose a reason for hiding this comment

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

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - ios DS - macos S - maintenance Repaying technical debt
Development

Successfully merging this pull request may close these issues.

None yet

4 participants