Page MenuHomePhabricator

c#: Implement IList<T> to Eina.List.
ClosedPublic

Authored by brunobelo on Tue, Nov 26, 1:35 PM.

Details

Summary

Container can have three configuration over Own and OwnContent:
Own = true and OwnContent = true;
Own = true and OwnContent = false;
Own = falseand `OwnContent = false;
If someone try to instanciate the container with Own = false and OwnContent = true, a exception raises.

There is two Ownerships' behaviours in c#, where IsReadOnly is responsible and IsReadOnly = !OwnContent:
Full Ownership: User can use modify/Add/Remove operations over the container, this is implemented with OwnContent = true.
No Ownership: User cannot use modify/Add/Remove operations, this is implemented with OwnContent = false.

For the memory, Own frees the node, while OwnContent frees the data portion.
ref T8487

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
brunobelo created this revision.Tue, Nov 26, 1:35 PM
brunobelo requested review of this revision.Tue, Nov 26, 1:35 PM
brunobelo updated this revision to Diff 27132.Tue, Nov 26, 1:43 PM

variable name wrong.

lauromoura requested changes to this revision.Tue, Nov 26, 7:35 PM

Overall looks good, just some issues.

src/bindings/mono/eina_mono/eina_list.cs
152–155

Is this needed?

Also aren't IList<T> supposed to be Read/Write? (In contrast with IReadOnlyList<T>.

448–449

What if n is negative?

625

Shouldn't we check for OwnContent here so we can free the data we're in charge?

670

What if idx is negative?

680

Ditto.

src/lib/eina/eina_list.c
649 ↗(On Diff #27132)

These changes on this file extracts this function the other two and have no direct impact on the IList<T> implementation, right?

It should be in a different patch, so we can discuss if this is actually needed.

src/tests/efl_mono/List.cs
10

To make sure it tests the IList<T> implementation, it could use IList<int> list = new Eina.List<int>();.

If any test code needs to access Eina.List-only methods then it'd use a normal cast.

20

Ditto.

39

Ditto

63

Ditto.

77

Ditto.

101

Ditto.

123

Ditto.

144

Ditto.

This revision now requires changes to proceed.Tue, Nov 26, 7:35 PM
brunobelo planned changes to this revision.Wed, Nov 27, 4:02 AM
brunobelo added inline comments.
src/bindings/mono/eina_mono/eina_list.cs
152–155

Yes, IList<T> need this variable and mono gives a error if not found it.

src/lib/eina/eina_list.c
649 ↗(On Diff #27132)

ok

lauromoura added inline comments.Wed, Nov 27, 5:03 AM
src/bindings/mono/eina_mono/eina_list.cs
152–155

Oh, right. Somehow I missed it in the IList<T> API page, sorry.

brunobelo updated this revision to Diff 27206.Fri, Nov 29, 6:51 AM

add more tests.

brunobelo planned changes to this revision.Tue, Dec 3, 5:30 AM
brunobelo updated this revision to Diff 27322.Wed, Dec 4, 12:29 PM

add new Ownerships.

Some comments.

Also, as discussed off-phab, from the point of view of the C# dev, the containers would be either read-only or fully writable, based on whether we own or not the contents. The ownership of of the container would be taken in account then only when freeing it.

src/bindings/mono/eina_mono/eina_list.cs
225

What is wild?

Also, value could be better named as something like shouldCheckBounds.

229

Instead of silently returning, methods that would be a NOP due to the list being read-only should raise https://docs.microsoft.com/en-us/dotnet/api/system.notsupportedexception?view=netframework-4.8

You are defining a general-purpose type with a state that enables operations conditionally. For example, your type can be either read-only or read-write. In that case:

If the object is read-only, attempting to assign values to the properties of an instance or call methods that modify instance state should throw a NotSupportedException exception.

289

Could check for OwnContent before this loop, so it can avoid multiple InternalNext calls?

359

C# will evaluate the first parameter before entering ModifyHandle regardless of IsReadOnly, causing a leak if the value is not appended (i.e. the list is read-only).

370

Ditto

384

Ditto

401

Ditto

456

Maybe we could hide this method, keeping only the indexer visible.

459–460

Also raise NotSupportedException as mentioned above.

lauromoura requested changes to this revision.Mon, Dec 9, 7:55 AM

Oops, forgot to request changes.

This revision now requires changes to proceed.Mon, Dec 9, 7:55 AM
brunobelo updated this revision to Diff 27431.Mon, Dec 9, 12:57 PM

updating Ownerships again

brunobelo edited the summary of this revision. (Show Details)Mon, Dec 9, 1:12 PM
brunobelo edited the summary of this revision. (Show Details)
brunobelo added a reviewer: jptiz.
brunobelo planned changes to this revision.Tue, Dec 10, 10:47 AM
jptiz added inline comments.Tue, Dec 10, 11:25 AM
src/bindings/mono/eina_mono/eina_list.cs
590–702

Same from Contains. In this case, cur could be initialized directly on for:

public int IndexOf(T val)
{
    var i = 0;
    for (IntPtr cur = Handle; cur != IntPtr.Zero; ++i, cur = InternalNext(cur))
    {
        if (NativeToManaged<T>(InternalDataGet(cur)).Equals(val))
        {
            return i;
        }
    }
    return -1;
}
646–658

I think comparing if a nullpointer has been reached inside for looks a bit confusing (at first I thought the method could end without return value), also giving the impression that there could be a nullpointer in the middle of the list.

Since for already has a "has it reached end?" syntax, I think this would made clearer that the whole list was checked and val wasn't found:

public bool Contains(T val)
{
    for (IntPtr cur = Handle; cur != IntPtr.Zero; cur = InternalNext(cur))
    {
        if (NativeToManaged<T>(InternalDataGet(cur)).Equals(val))
        {
            return true;
        }
    }
    return false;
}
brunobelo updated this revision to Diff 27484.Wed, Dec 11, 6:26 AM

removing unused method

lauromoura accepted this revision.Wed, Dec 11, 1:58 PM
This revision is now accepted and ready to land.Wed, Dec 11, 1:58 PM
This revision was automatically updated to reflect the committed changes.

Aaaaaand it's gone