Skip to content

provided toString() because strlen must die#8550

Merged
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:strlen-must-die
Aug 9, 2018
Merged

provided toString() because strlen must die#8550
dlang-bot merged 1 commit intodlang:masterfrom
WalterBright:strlen-must-die

Conversation

@WalterBright
Copy link
Member

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#8550"

Copy link
Contributor

@JinShil JinShil left a comment

Choose a reason for hiding this comment

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

You could replace a toChars or two to get full test coverage.

@Geod24
Copy link
Member

Geod24 commented Aug 9, 2018

SemaphoreCI was stuck.
I think it'll be better to store the length instead of calling strlen on every toString, but it's already an improvement.

@dlang-bot dlang-bot merged commit 72f842b into dlang:master Aug 9, 2018
@jacob-carlborg
Copy link
Contributor

Here new public symbols were added without Ddoc 😞.

@jacob-carlborg
Copy link
Contributor

How will this affect the C++ headers?

@JinShil
Copy link
Contributor

JinShil commented Aug 9, 2018

Here new public symbols were added without Ddoc 😞.

Have you ever tried to convince @WalterBright to do things like this? It's better if we just merge and add the docs ourselves in a separate PR.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Aug 9, 2018

We managed to get him to do PRs instead of pushing directly. Also he’s the one nagging everybody else to have proper Ddoc sections.

@WalterBright
Copy link
Member Author

How will this affect the C++ headers?

It won't. They are not callable from C++, and they don't occupy a slot in the vtbl[].

without Ddoc

If it did something different than every other toString() in dmd, then it would need a Ddoc. Actually, if it did something different than every other toString(), it should have a different name as well as a Ddoc.

As always, use your best judgement.

@WalterBright WalterBright deleted the strlen-must-die branch August 9, 2018 20:05
@WalterBright
Copy link
Member Author

I think it'll be better to store the length instead of calling strlen on every toString, but it's already an improvement.

It may be. But by adding this function, the need for strlen becomes encapsulated, and is not leaked to the users of the object. A sign of effective encapsulation is being able to change the underlying data structure without needing to change any users of the object.

@jacob-carlborg
Copy link
Contributor

It won't. They are not callable from C++, and they don't occupy a slot in the vtbl[].

👍.

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.

6 participants