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.

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

Get size of map and regex captures #956

Merged
merged 6 commits into from
May 31, 2021

Conversation

ALANVF
Copy link
Contributor

@ALANVF ALANVF commented Apr 22, 2021

Part 2 of an effort to add methods to get the size of a map, and the number of captures from a regex.

There are other ways I could implement the size method for Hashes, so I can do it another way if you don't like my current implementation.

Option 1

The current solution, which is to cast the XXXHashBase * value to XXXHashObject * and access getSize that way.

int __XXX_hash_size(Dynamic &ioHash) {
    XXXHashBase *hash = static_cast<XXXHashBase *>(ioHash.GetPtr());
    if(!hash)
       return 0;
    return ((XXXHashObject *) hash)->getSize();
}

Option 2

Add HashBase#getSize() as a virtual method, and remove the inline qualifier from the Hash#getSize() method.

class HashBase ... {
    ...
    virtual int getSize() = 0;
};

class Hash ... {
    ...
    int getSize() { return size; }
};

Option 3

Instead of making getSize a virtual method, add a variant of it that is virtual instead (that way it doesn't affectexisting code).

class HashBase ... {
    ...
    virtual int getDynSize() = 0;
};

class Hash ... {
    ...
    int getDynSize() { return size; }
};

Option 4

Move the size property from Hash to HashBase, and then move getSize from Hash to HashBase so that it can remain inline.

class HashBase ... {
    int size;
    ...
    inline int getSize() { return size; }
};

class Hash ... {
    ...
};

Part 1: HaxeFoundation/hashlink#437

@hughsando
Copy link
Member

I think option 1/4 can work. There is already "struct HashRoot" which can form this base class.
The "size" member can be moved into this struct (so could "mask" and "bucketCount")
The dynamic hash could then be static-cast to HashRoot and you would not need a separate function for each type.

@ALANVF
Copy link
Contributor Author

ALANVF commented May 8, 2021

Status?

@hughsando
Copy link
Member

Looks good, but can you remove the unused type-specific size functions from the header?
Also, the base variables can be exposed in the derived class with using HashRoot::size etc in the Hash class, rather than with the this-> notation.

@ALANVF
Copy link
Contributor Author

ALANVF commented May 30, 2021

Fixed 👍

@hughsando hughsando merged commit b797ae4 into HaxeFoundation:master May 31, 2021
@hughsando
Copy link
Member

Brilliant! Thanks a lot.

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.

2 participants