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

Refactor element/text base class #359

Closed

Conversation

teleological
Copy link
Collaborator

Factored out common methods in Element/Text. Couldn't come up with a decent name for the result; would welcome suggestions.

Perhaps more importantly: Noticed some memory issues when adding text siblings. It may be good to patch those before letting this out into the wild. Working on a fix, but wanted to get a clean refactor out first. ( Bug fixes in #362 )

@teleological teleological changed the title Factor element, node into fraternal_node class Refactor element/text base class, avoid segfault on merge Nov 15, 2015
@teleological
Copy link
Collaborator Author

Renamed "fraternal" nodes to "non-attribute nodes", updated XmlComment to be a subclass, and changed XmlNode::New so that it creates the correct kind of subclasses (fixes #226)

@rchipka
Copy link
Member

rchipka commented Nov 16, 2015

It could also be called a proto-element or ProtoElement type. Also, in #360 the add_child, add_next_sibling, add_prev_sibling, and import_element functions have all been moved to XmlNode. The reason I moved these to XmlNode is so that the refcount logic is all in one location. This way, anything that inherits from XmlNode will have these functions available, but won't have them exposed via the API unless the XmlNode inheritor explicitly implements addChild(), etc.

We could easily move that same logic into this sub class instead, just as long as it's in one place.

@teleological
Copy link
Collaborator Author

@rc0x03 I also moved import_element to XmlNode, but didn't move the others because they didn't seem a good fit with XmlAttribute, which also descends from node. Have you looked into that? I've started to review #360.

@teleological
Copy link
Collaborator Author

I'm going to extract the memory fix for adding/merging text nodes into a separate PR that can be merged without delay. I will leave the refactoring of the Element/Text base class for later consideration. The correction of the wrapper selection for child nodes, etc. is technically a bug fix, but it also impacts usage, since nodes returned from some operations won't respond to the same API as before the change. So it would be kinder not to shoehorn it into a minor release.

@teleological teleological mentioned this pull request Nov 16, 2015
@teleological
Copy link
Collaborator Author

Bug fixes extracted to #362. Once that is merged, will rebase this PR to exclude those commits.

@teleological teleological changed the title Refactor element/text base class, avoid segfault on merge Refactor element/text base class Nov 16, 2015
@rchipka rchipka closed this Mar 29, 2023
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.

None yet

2 participants