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

Update hl._std.sys.thread.Lock #8744

Merged
merged 6 commits into from
Nov 28, 2019
Merged

Update hl._std.sys.thread.Lock #8744

merged 6 commits into from
Nov 28, 2019

Conversation

ConstNW
Copy link
Contributor

@ConstNW ConstNW commented Sep 2, 2019

haxe implementation of the lock bindings(HaxeFoundation/hashlink/pull/292) suggested by @ncannasse at #8699

@Aurel300
Copy link
Member

Aurel300 commented Sep 2, 2019

Please remove the docstrings from the hl implementation.

class Lock {
var deque:sys.thread.Deque<Bool>;
extern class Lock {
public function new():Void;
Copy link
Member

Choose a reason for hiding this comment

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

public is redundant in externs.

@ncannasse
Copy link
Member

This cannot be merged as-it as we need to be able to set the minimal HL version to 1.11, the way we did for Date utc_inf

@ConstNW
Copy link
Contributor Author

ConstNW commented Nov 27, 2019

@ncannasse is it ready to merge?

@ncannasse
Copy link
Member

I think there's still an issue given for documentation generation since we cannot have a std class that is sometimes an abstract on some platforms. please confirm @RealyUniqueName

@RealyUniqueName
Copy link
Member

Yeah, that's bad. Especially given that in the doc we will have it listed as a class, but in an actuall code it will be an abstract.

I suggest to change it to something like this:

class Lock {
  var native:hl.Abstract<"hl_lock">;
  <...>
}

@ncannasse ncannasse merged commit 140842a into HaxeFoundation:development Nov 28, 2019
@ncannasse
Copy link
Member

@RealyUniqueName ok for 4.0 branch

@ConstNW ConstNW deleted the patch-2 branch November 28, 2019 13:08
RealyUniqueName pushed a commit that referenced this pull request Nov 28, 2019
@RealyUniqueName
Copy link
Member

Done

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