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

Remove a level of indirection from internal std.xml.ElementParser code #5380

Merged
merged 1 commit into from
May 11, 2017

Conversation

JackStouffer
Copy link
Member

Removes an odd choice to keep the base string of a tag as a pointer to a string rather than the string itself.

I don't see an obvious reason why this was done, so I removed it for code simplicity and possible speed improvements.

@quickfur
Copy link
Member

A pointer to a string might be necessary if the caller expects the callee to modify the string (as in, replace it with a different slice of the original data).

@quickfur
Copy link
Member

In modern D we'd pass the string by ref, but this code may predate that, I'm not sure.

@quickfur
Copy link
Member

But if the autotester passes, that probably means it was completely unnecessary and we can just merge this. :-D

@JackStouffer
Copy link
Member Author

As best as I can tell, it looks like an attempt to avoid copying. Which is ridiculous as the string struct is tiny in reality and you end up having a pointer to a pointer for the actual data of the string, which is very bad for caches.

@quickfur
Copy link
Member

I say we just merge this and see what explodes. We can clean up later. This code is badly in need of a major cleanup, new std.xml candidates notwithstanding.

@JackStouffer
Copy link
Member Author

@quickfur Thanks. For more std.xml clean up please consider looking at #5378

@dlang-bot dlang-bot merged commit 0b916c9 into dlang:master May 11, 2017
@JackStouffer JackStouffer deleted the xml-pointer branch May 11, 2017 20:10
@aG0aep6G
Copy link
Contributor

This introduced a regression. See #5489.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants