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

Fix issue 18632 - enable use of fromStringz with char[n] #8164

Merged
merged 1 commit into from Jul 16, 2021

Conversation

dkorpel
Copy link
Contributor

@dkorpel dkorpel commented Jul 11, 2021

No description provided.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @dkorpel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

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 references

Auto-close Bugzilla Severity Description
18632 enhancement enable use of fromStringz with char[n]

Testing this PR locally

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

dub run digger -- build "master + phobos#8164"

@RazvanN7
Copy link
Collaborator

LGTM

@dkorpel dkorpel marked this pull request as ready for review July 13, 2021 09:16
@RazvanN7
Copy link
Collaborator

One observation is that fromStringz was originally designed to convert a C string (null terminated char*) to a D string (char[]). Since a static array is already a D string, it can be argued that allowing the case that this PR enables is pointless. However, I think this enables the use of fromStringz in generic code where it is not obvious whether the string is a C string or a D string. As such, I don't have anything against this addition, however, I'll wait for others to weight in.

@RazvanN7 RazvanN7 added 72h no objection -> merge The PR will be merged if there are no objections raised. Atila Neves labels Jul 13, 2021
@dkorpel
Copy link
Contributor Author

dkorpel commented Jul 13, 2021

Since a static array is already a D string, it can be argued that allowing the case that this PR enables is pointless.

The use case I have in mind is C api's using structures with fixed size strings, e.g:

struct WIN32_FIND_DATAW {
    DWORD           dwFileAttributes;
    FILETIME        ftCreationTime;
    // (...)
    WCHAR[MAX_PATH] cFileName = 0;
}

With the new overload, you can safely extract cFileName.fromStringz instead of cFileName.ptr.fromStringz.

@Geod24
Copy link
Member

Geod24 commented Jul 15, 2021

Additionally, some C API accept a pointer and a length and will write to that. E.g. getcwd. In which case, one might want to use a static array to avoid GC allocations.

@RazvanN7 RazvanN7 merged commit cf83da3 into dlang:master Jul 16, 2021
@dkorpel dkorpel deleted the fromstringz-array branch July 16, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
72h no objection -> merge The PR will be merged if there are no objections raised. Atila Neves Enhancement
Projects
None yet
4 participants