Skip to content

Fix edge metadata key when cloning - #153

Open
silas-martens wants to merge 2 commits into
tudasc:develfrom
silas-martens:fix/MDCloneKey
Open

Fix edge metadata key when cloning#153
silas-martens wants to merge 2 commits into
tudasc:develfrom
silas-martens:fix/MDCloneKey

Conversation

@silas-martens

Copy link
Copy Markdown
Contributor

When merging edge metadata, clone() returns a std::unique_ptr<MetaData>. Consequently, template argument deduction for addEdgeMetaData() selects T = MetaData, causing T::key to resolve to MetaData::key ("BaseClass") rather than the key of the dynamic metadata type.

I noticed this when cgmerge2 changed stored my previously correct metadata under the "BaseClass" key.

I fixed this, by using the virtual getKey() method when inserting edge metadata.

@pearzt pearzt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM. I'm surprised that this was not caught by tests. Could you take a look into the tests and make sure this is covered?

@TimHeldmann

Copy link
Copy Markdown
Member

It is a shame, that C++ does not support covariant return-types for unique_ptr specializations.
So I think this fix is valid, if we do not want to go back to raw pointers for the cloning functions, or want to introduce an additional CRTP layer to the metadata registry.

If you are able to add a test, this looks good to me

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