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

WIP: Remove warnings #102

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

WIP: Remove warnings #102

wants to merge 4 commits into from

Conversation

niil-ohlin
Copy link
Contributor

@niil-ohlin niil-ohlin commented Mar 30, 2020

A solution for removing warnings. I really don't like this solutions. It's way to naive and hacky and really degrades the quality of the codebase. Does anyone have a suggestion for how the mutex warnings can be addressed?

Copy link
Contributor

@moglistree moglistree left a comment

Choose a reason for hiding this comment

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

Can we rebase this on the merged 5.2 for starters?

.circleci/config.yml Outdated Show resolved Hide resolved
.circleci/config.yml Outdated Show resolved Hide resolved
@@ -21,20 +21,22 @@ public final class Callbacker<Value> {

private var callbacks = Callbacks.none
private var _mutex = pthread_mutex_t()
private var mutex: PThreadMutex { return PThreadMutex(&_mutex) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you allowed to add extension to PThreadMutex (UnsafeMutablePointer<pthread_mutex_t>)? Otherwise perhaps create a thin wrapper around it and use that instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea! I'll try that!

@niil-ohlin niil-ohlin force-pushed the remove-warnings branch 3 times, most recently from 0397903 to d26589b Compare March 30, 2020 10:48
@niil-ohlin niil-ohlin changed the title WIP WIP WIP: Remove warnings WIP: Remove warnings Mar 30, 2020
}

mutating func unlock() {
withPointer { $0.unlock() }
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not add protect as well, to avoid rewriting a lot of code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It caused crashes when I used protect. I'm not a 100% sure why though.

Copy link
Contributor

Choose a reason for hiding this comment

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

How did your code look like and did the log specify why it crashed (e.g. exclusivity violation)?

@@ -10,8 +10,7 @@ import Foundation

/// A reference wrapper around a POSIX thread mutex
public final class Mutex {
private var _mutex = pthread_mutex_t()
private var mutex: PThreadMutex { return PThreadMutex(&_mutex) }
private var mutex = pthread_mutex_t()
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this comment:

/// Helper methods to work directly with a Pthread mutex pointer to avoid overhead of alloction and reference counting of using the Mutex reference type.
/// - Note: You have to explicity call `initialize()` before use (typically in a class init) and `deinitialize()` when done (typically in a class deinit)
extension UnsafeMutablePointer where Pointee == pthread_mutex_t {

I'm trying to remember what the reason we the pthread_mutex_t -> PThreadMutex dance above, and if this change will alter the behaviour are performance...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, ok this solution might not be correct at all. I thought that because private var mutex: PThreadMutex { return PThreadMutex(&_mutex) } would create a new UnsafeMutablePointer every time you called it. It would be functionally the same as withUnsafeMutablePointer(to: &_mutex) { ... } but it seems to change behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

@niil-ohlin do you remember why this didn't work? I opened #104 now and saw your PR

@moglistree moglistree force-pushed the master branch 2 times, most recently from deccedd to 65d4f58 Compare February 15, 2023 08:54
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.

5 participants