Page MenuHomePhabricator

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

Authored by brunobelo on Nov 26 2019, 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
Branch
list
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 14689
Build 10114: arc lint + arc unit
brunobelo created this revision.Nov 26 2019, 1:35 PM
brunobelo requested review of this revision.Nov 26 2019, 1:35 PM
brunobelo updated this revision to Diff 27132.Nov 26 2019, 1:43 PM

variable name wrong.

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

Overall looks good, just some issues.

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

Is this needed?

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

374

What if n is negative?

554

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

624

What if idx is negative?

634

Ditto.

src/lib/eina/eina_list.c
649

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
9

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.

19

Ditto.

38

Ditto

62

Ditto.

76

Ditto.

100

Ditto.

122

Ditto.

143

Ditto.

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

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

src/lib/eina/eina_list.c
649

ok

lauromoura added inline comments.Nov 27 2019, 5:03 AM
src/bindings/mono/eina_mono/eina_list.cs
149

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

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

add more tests.

brunobelo planned changes to this revision.Dec 3 2019, 5:30 AM
brunobelo updated this revision to Diff 27322.Dec 4 2019, 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
184

What is wild?

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

188

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.

227

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

300

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).

307

Ditto

315–316

Ditto

327

Ditto

387

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

397

Also raise NotSupportedException as mentioned above.

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

Oops, forgot to request changes.

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

updating Ownerships again

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

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;
}
575–587

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.Dec 11 2019, 6:26 AM

removing unused method

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

Aaaaaand it's gone