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.

Sign up for GitHub

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

Jump to bottom

Attach finalizers to LibGit2 methods #19660

Merged
merged 7 commits into from
Jan 9, 2017
Merged

Attach finalizers to LibGit2 methods #19660

merged 7 commits into from
Jan 9, 2017

Conversation

simonbyrne
Copy link
Contributor

Fixes #17414.

@simonbyrne simonbyrne added the libgit2 The libgit2 library or the LibGit2 stdlib module label Dec 20, 2016
@simonbyrne
Copy link
Contributor Author

The alternative here, of course, is to actually add these methods as finalizers to the relevant objects: this would probably make more sense in the long run (as a benefit, we wouldn't need to then deprecate the finalize methods, as these would be implicit).

tkelman
tkelman approved these changes Dec 20, 2016
View reviewed changes
Copy link
Contributor

@tkelman tkelman left a comment

Choose a reason for hiding this comment

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

this lgtm for now. be sure to squash

@simonbyrne
Copy link
Contributor Author

@wildart Do you know why we don't use object finalizers for these?

@simonbyrne simonbyrne force-pushed the sb/libgit2/no-finalize branch from ace5716 to f790e88 Compare December 30, 2016 23:25
@simonbyrne
Copy link
Contributor Author

I've cleaned this up so that most now actually act like finalizers. This can't be used for StrArrayStruct and Buffer though, as they're immutable, but these aren't really user facing.

@tkelman
Copy link
Contributor

tkelman commented Dec 31, 2016

seems to cause Pkg issues, see the travis log

@simonbyrne simonbyrne force-pushed the sb/libgit2/no-finalize branch from fdd25fa to ead1e44 Compare January 4, 2017 12:41
@simonbyrne
Copy link
Contributor Author

Okay, hopefully this should be good.

@simonbyrne simonbyrne changed the title Overload Base.close instead of Base.finalize for LibGit2 Attach finalizers to LibGit2 methods Jan 4, 2017
@simonbyrne simonbyrne mentioned this pull request Jan 4, 2017
Open
42 tasks
tkelman
tkelman reviewed Jan 4, 2017
View reviewed changes
base/libgit2/types.jl
ccall((:git_object_free, :libgit2), Void, (Ptr{Void},), obj.ptr)
obj.ptr = C_NULL

macro defclose(typ,cname)
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, no: was left over from my refactoring

tkelman
tkelman reviewed Jan 4, 2017
View reviewed changes
base/libgit2/types.jl
(:GitTree, :GitRepo, :GitObject, :git_tree),
(:GitTag, :GitRepo, :GitObject, :git_tag)]

if reporef == nothing
Copy link
Contributor

Choose a reason for hiding this comment

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

===

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@simonbyrne simonbyrne force-pushed the sb/libgit2/no-finalize branch from ead1e44 to 455063b Compare January 4, 2017 13:42
@simonbyrne simonbyrne force-pushed the sb/libgit2/no-finalize branch from 455063b to 4bf63fe Compare January 7, 2017 14:21
tkelman
tkelman reviewed Jan 7, 2017
View reviewed changes
base/libgit2/libgit2.jl
end
[GitAnnotated(repo, tr_brn_ref)]
end
tr_brn_ref = upstream(head_ref)
Copy link
Contributor

@tkelman tkelman Jan 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

upstream(head(repo)) ? does this not need with(head(repo)) any more?

Copy link
Contributor Author

@simonbyrne simonbyrne Jan 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, something got broken in the rebase. I'll try to figure out what happened.

@simonbyrne
Copy link
Contributor Author

Okay, I think this is ready.

@tkelman tkelman merged commit db4aaa3 into master Jan 9, 2017
@tkelman tkelman deleted the sb/libgit2/no-finalize branch January 9, 2017 10:16
@KristofferC
Copy link
Member

KristofferC commented Jan 9, 2017 •
edited
Loading

Are there any requirements from libgit2 that objects should be freed "quickly" as they are when using the with form? Or is it fine to just delay the freeing until whenever the finalizers are run?

@simonbyrne
Copy link
Contributor Author

Not that I can tell, no.

In fact, we could probably clean up the code quite a bit by getting rid of all the with statements.

@tkelman
Copy link
Contributor

tkelman commented Jan 9, 2017

Unless I guess anything on the C side holds open file handles that we'd like to delete or similar?

@simonbyrne
Copy link
Contributor Author

That could be, but the libgit2 docs are pretty sparse in that regard.

@simonbyrne simonbyrne mentioned this pull request Jan 18, 2017
Closed
@Sacha0 Sacha0 added deprecation This change introduces or involves a deprecation needs news A NEWS entry is required for this change labels May 20, 2017
@Sacha0 Sacha0 mentioned this pull request May 20, 2017
Closed
@KristofferC KristofferC removed the needs news A NEWS entry is required for this change label Nov 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tkelman tkelman tkelman approved these changes

Assignees
No one assigned
Labels
deprecation This change introduces or involves a deprecation libgit2 The libgit2 library or the LibGit2 stdlib module
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simonbyrne @tkelman @KristofferC @Sacha0