NotificationsYou must be signed in to change notification settings
Fork7
Star5

Adds an unchecked version to get a BorrowedReference pointer#4

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

amos402 merged 9 commits intoamos402:soft-shutdownfromUnity-Technologies:soft-shutdown-review-comments-3

Sep 29, 2020

Merged

Adds an unchecked version to get a BorrowedReference pointer#4

amos402 merged 9 commits intoamos402:soft-shutdownfromUnity-Technologies:soft-shutdown-review-comments-3

Sep 29, 2020

Conversation

Copy link

BadSingleton commentedSep 15, 2020

And other code review changes.

BadSingletonand others added5 commits

September 15, 2020 14:31

Adds an unchecked version to get a BorrowedReference pointer

6a3cfc8

And other code review changes

Remove compile-time check on NETSTANDARD

98da1fa

Even if netstandard lacks (for now) the necessary APIs for domain(un)loading, it is still possible to (un)load domains via the Mono Cruntime.

fixup! Remove compile-time check on NETSTANDARD

d7d44e8

Factor out the clearing of clr_data

6aa75c5

In Runtime_data.cs

Partially Revert "Adds an unchecked version to get a BorrowedReferenc…

2343f89

…e pointer"We Don't support python 2 anymore, but the CI machines may still beusing it to build.

Copy link

codecov-commenter commentedSep 16, 2020•
edited
Loading

Codecov Report

Merging#4 intosoft-shutdown willnot change coverage.
The diff coverage isn/a.

@@              Coverage Diff               @@##           soft-shutdown       #4   +/-   ##==============================================  Coverage          86.25%   86.25%           ==============================================  Files                  1        1             Lines                291      291           ==============================================  Hits                 251      251             Misses                40       40

Flag	Coverage Δ
#setup_linux	`64.94% <ø> (ø)`
#setup_windows	`72.50% <ø> (ø)`

Flags with carried forward coverage won't be shown.Click here to find out more.

Continue to review full report at Codecov.

Legend -Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing data
Powered byCodecov. Last update12c0206...ff956e4. Read thecomment docs.

lostmsu reviewed

Sep 24, 2020

View reviewed changes

src/runtime/runtime_data.cs Outdated

Comment on lines 39 to 45

		IntPtrcapsule=PySys_GetObject("clr_data").DangerousGetAddressUnchecked();
		if(capsule!=IntPtr.Zero)
		{
		IntPtroldData=PyCapsule_GetPointer(capsule,null);
		PyMem_Free(oldData);
		PyCapsule_SetPointer(capsule,IntPtr.Zero);
		}

Copy link

lostmsuSep 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Now that you never own the pointer, you can useBorrowedReference and never call "Dangerous" methods on it. MakePyCapsule_GetPointer andPyCapsule_SetPointer take first argument asBorrowedReference.

Copy link

Author

BadSingletonSep 24, 2020

Choose a reason for hiding this comment

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

Make PyCapsule_GetPointer and PyCapsule_SetPointer take first argument as BorrowedReference

I don't think this is semantically correct. These methods can operate on something other than BorrowedReferences

Copy link

lostmsuSep 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

No, this is exactly right. The method borrows a capsule object in a similar way we use borrowed references. The second argument of course should stay as raw pointer.

Think of it this way: if you were to implementPyCapsule_GetPointer, you'd need a borrowed reference to the capsule in order to update it (as opposed to stealing or taking/giving up ownership).

Copy link

Author

BadSingletonSep 25, 2020

Choose a reason for hiding this comment

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

In this use case, it is. However in the case where I create the new capsule (and receive aNewReference), and then set the capsule's pointer withPyCapsule_SetPointer, well.. it's a bit weird. But as I said in another reply,NewReference in implicitly castable to borrowed reference, which makes it transparent to the user.

lostmsu reviewed

Sep 24, 2020

View reviewed changes

src/runtime/runtime_data.cs Outdated

		privatestaticvoidRestoreRuntimeDataImpl()
		{
		IntPtrcapsule=PySys_GetObject("clr_data").DangerousGetAddress();
		IntPtrcapsule=PySys_GetObject("clr_data").DangerousGetAddressUnchecked();

Copy link

lostmsuSep 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Same here, you don't need the raw address.

Copy link

Author

BadSingletonSep 24, 2020

Choose a reason for hiding this comment

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

I do need it.PyCapsule_GetPointer at line 113 needs an IntPtr. Sure I could add an overload forPyCapsule_GetPointer, but I think what we really need is an implicit operator.

lostmsu reviewed

Sep 24, 2020

View reviewed changes

src/runtime/runtime_data.cs Outdated

		}
		capsule=PyCapsule_New(mem,null,IntPtr.Zero);
		ClearCLRData();
		IntPtrcapsule=PyCapsule_New(mem,null,IntPtr.Zero);

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

Ideally, this should have beenNewReference, and decref below should beDispose

More changes to use BorrowedReference and NewReference

cc6b8e4

Also adds implicit IntPtr conversion operators to simplify their use.

lostmsu suggested changes

Sep 24, 2020

View reviewed changes

src/runtime/BorrowedReference.cs Outdated

Comment on lines 13 to 14

		publicstaticimplicitoperatorIntPtr(inBorrowedReferenceself)=>self.DangerousGetAddress();

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

This exposes dangerous shotgun as an innocent operator

src/runtime/NewReference.cs Outdated

		=>newBorrowedReference(reference.pointer);

		[Pure]
		publicstaticimplicitoperatorIntPtr(inNewReferencereference)

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

Same

src/runtime/runtime.cs Outdated

Comment on lines 751 to 766

		/// <remark>
		/// We need this method because BorrowedReference can be implicitly casted to IntPtr.
		/// </remark>
		internalstaticvoidXDecref(BorrowedReferenceop)
		{
		thrownewInvalidOperationException("Cannot DecRef a borrowed reference.");
		}

		/// <remark>
		/// We need this method because NewReference can be implicitly casted to IntPtr.
		/// </remark>
		internalstaticvoidXDecref(NewReferenceop)
		{
		op.Dispose();
		}

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

No implicit casts. The types were created specifically to avoid freely mixing different kinds of ownership forPyObject*.

src/runtime/runtime.cs


		[DllImport(_PythonDll, CallingConvention= CallingConvention.Cdecl)]
		internalstaticexternIntPtr PyCapsule_New(IntPtr pointer,string name, IntPtr destructor);
		internalstaticexternNewReference PyCapsule_New(IntPtr pointer,string name, IntPtr destructor);

Copy link

lostmsuSep 24, 2020•
edited
Loading

Choose a reason for hiding this comment

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

Re: implicit conversions toIntPtr: If you find it hard to change all the places, that usePyCapsule_New to useNewReference avoiding its dangerous methods as much as possible, just keep this return typeIntPtr here, and useNewReference at the call site.

Copy link

Author

BadSingletonSep 24, 2020

Choose a reason for hiding this comment

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

Not that I find it hard, I got confused by how theBorrowedReference gets marshalled. Or more like how it could get marshalled without needing an overload.
Also, NewReference is implicitly castable toBorrowedReference which solves most issues. The naming still bugs me a bit, but if it works, it works.

src/runtime/runtime.cs Outdated

		internalstaticexternNewReference PyCapsule_New(IntPtr pointer,string name, IntPtr destructor);

		[DllImport(_PythonDll, CallingConvention= CallingConvention.Cdecl)]
		internalstaticextern IntPtr PyCapsule_GetPointer(IntPtr capsule,string name);

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

This should beIntPtr PyCapsule_GetPointer(BorrowedReference capsule, string name)

Copy link

lostmsuSep 24, 2020

Choose a reason for hiding this comment

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

Or you can add an overload.

BadSingleton added2 commits

September 24, 2020 16:10

fixup! More changes to use BorrowedReference and NewReference

d5fcfa4

fixup! fixup! More changes to use BorrowedReference and NewReference

fa47957

Copy link

Author

BadSingleton commentedSep 25, 2020

travis-ci has one failure on an unrelated error:

CSC : error CS0009: Metadata file '/home/travis/.nuget/packages/microsoft.targetingpack.netframework.v4.5/1.0.1/lib/net45/System.Xml.dll' could not be opened -- Unexpected stream end. [/home/travis/build/amos402/pythonnet/src/testing/Python.Test.15.csproj]

lostmsu approved these changes

Sep 25, 2020

View reviewed changes

Copy link

lostmsu left a comment

Choose a reason for hiding this comment

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

Looks good to me. The newDangerous method is not used anywhere though, so should not be added.

src/runtime/BorrowedReference.cs Outdated

Comment on lines 18 to 22

		/// <summary>
		/// Gets a raw pointer to the Python object. Does not throw an exception
		/// if the pointer is null
		/// </summary>
		publicIntPtrDangerousGetAddressUnchecked()=>this.pointer;

Copy link

lostmsuSep 25, 2020

Choose a reason for hiding this comment

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

NIT: I don't think you are actually using this anywhere

Copy link

Author

BadSingletonSep 25, 2020

Choose a reason for hiding this comment

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

Indeed, it's not used anymore

Remove unused method

ff956e4

amos402 merged commitc7b134c intoamos402:soft-shutdown

Sep 29, 2020

BadSingleton deleted the soft-shutdown-review-comments-3 branch

October 20, 2020 14:59

Labels

None yet

Movatterモバイル変換

Adds an unchecked version to get a BorrowedReference pointer#4

Adds an unchecked version to get a BorrowedReference pointer#4

Uh oh!

Conversation

BadSingleton commentedSep 15, 2020

Uh oh!

codecov-commenter commentedSep 16, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

lostmsuSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lostmsuSep 24, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BadSingleton commentedSep 25, 2020

Uh oh!

lostmsu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commentedSep 16, 2020•
edited
Loading

lostmsuSep 24, 2020•
edited
Loading

lostmsuSep 24, 2020•
edited
Loading

lostmsuSep 24, 2020•
edited
Loading

lostmsuSep 24, 2020•
edited
Loading