-
-
Notifications
You must be signed in to change notification settings - Fork 746
Add optional parameter appendNullTerminator
to read
and readText
#10856
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request and interest in making D better, @nordlow! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please see CONTRIBUTING.md for more information. If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment. Bugzilla referencesYour 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 locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub run digger -- build "master + phobos#10856" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You appear to have converted all spaces to tabs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please also add the new parameter to the documentation comments
Need to remove whitespace changes. Hang on. |
a760bd2
to
1a9765e
Compare
Hi @nordlow , Would be interesting to know so that we can evaluate adding this:
@thewilsonator I think addition of new parameters which implement new features should be treated in the same way as addition of new symbols. Does that still need BDFL approval? |
One way we can approach this is to just do this unconditionally; then, we don't need a new option to take up space in the argument list. It would mean allocating one extra byte when reading files, which is a negligible overhead. For empty files, we can return |
Agreed. Turns out the extra byte at the end is already allocated in the master version of readImpl. So then we just need to document this behavior and add a test verifying the null terminator at the end. |
The motive for adding the null at the end is that it enables sentinel-based search in lexers. Being somewhat of a forgotten technique for increasing the speed of lexers and in turn parsers and scanners. Andrei Alexandrescu has talked about this previously on at least occation. I believe he used the term "sentinel-based search", IRC. FYI, @andralex. |
The whitespace changes make it nigh impossible to find where the actual changes are, so I don't know what the diff should be. Having said, the title of the MR makes me question why this would be desired. |
Did you read #10856 (comment)? |
Yes, but why can't the user append it if they want to? |
Can and should are two different things. If you do it when you're dealing with buffers like read/readText should be, it can be free. If the user does it outside of the buffer handling, that is very much not free. |
No description provided.