Skip to content

Implemented IDisposable and Dispose pattern #10

Merged
dmedine merged 1 commit intolabstreaminglayer:masterfrom
GrahamBriggs:IDisposable
Mar 30, 2021
Merged

Implemented IDisposable and Dispose pattern #10
dmedine merged 1 commit intolabstreaminglayer:masterfrom
GrahamBriggs:IDisposable

Conversation

@GrahamBriggs
Copy link
Copy Markdown
Contributor

You can now close an outlet by disposing of it (instead of waiting for garbage collection to close it for you)

…rap unmanaged resources

You can now close an outlet by disposing of it (instead of waiting for garbage collection to close it for you)
@cboulay
Copy link
Copy Markdown
Contributor

cboulay commented Mar 29, 2021

Great, thanks for redoing this. Based on your comments in #9 , I'm satisfied.

I won't merge it myself because I think David and/or Tristan should add a version somewhere and tag that commit before merging, then after merging bump the version number and tag the commit with an incremental number. I would do it myself but I don't think there was an agreement to where the version number should go.

@dmedine
Copy link
Copy Markdown
Contributor

dmedine commented Mar 30, 2021

Yes, also thanks from me for rebasing. I will pull this and add a version function. This is now part of this issue, so if we are going to futz with the namespace and the class naming, I would actually remove the outer class liblsl (which should have been static) altogether and just rely on the LSL namespace. The 'helper' functions (of which we will add 'wrapper_version', which is no more of a runtime collision threat than is the already existing 'library_version'---especially since it is (ahem) incorrect to use underscores when naming methods in C#) are all static already anyway and this also keeps the wrapper consistent with the C++ API which has the same structure. Then you would have var streamOutlet = LSL.StreamOutlet(...) etc.

It begs the question, though, if the namespace should be consistent with C++, i.e. 'lsl' not 'LSL'. That we follow C# conventions for some things but not others annoys me and makes me embarrassed to release this as a NuGet package.

I would guess that the reason the C# implementation is a bit feeble is that it was done very hastily in the very beginning and that Christian didn't know about the need for using IDisposable back then. Indeed most of the typical LSL use cases don't require IDisposable so it makes sense that initial tests didn't trigger GC madnesses.

If we want to get crazy, we could also make the methods with timeouts utilize async, but in this case, we would want to update the target to .NET Core>=3.0 because many of the good things for async patterns came in with C# 8.0---but that really is another issue. For now, @tstenner please comment on my namespace proposal. When we reach a consensus I will just commit it to this PR and we are done with this. I will then pop open a new branch to deal with the white space and the comments.

@dmedine dmedine merged commit 3fe3201 into labstreaminglayer:master Mar 30, 2021
@dmedine
Copy link
Copy Markdown
Contributor

dmedine commented Mar 30, 2021

Chad suggested going ahead with the merge. We will add the version method in a new PR.

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.

3 participants