Fix Issue 19133 - core.exception.rangeerror@std/file.d(3812)

[edi33416]

No description provided.

[dlang-bot]

Thanks for your pull request and interest in making D better, @edi33416! 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 annotated coverage diff directly on GitHub with CodeCov's browser extension
• 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
✓	19133	regression	core.exception.rangeerror@std/file.d(3812):

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 "stable + phobos#6805"

[CyberShadow]

How many times are we going to need to reinvent strlen? This is a misguided endeavor causing too many regressions in a too tiny fragment of code.

The commit that started it all, 5e88b67, is also not truthful in its commit message, as it was more than a refactoring and changed behavior.

I suggest the following fix:

• On systems where dirent has a d_reclen d_namlen field (which is not just Solaris), use that. See d_type for an example.
• On other systems, use strlen (with d_name.ptr). It is a C API, it is what programs written in C are supposed to do, so trust the OS and follow the spec.

[CyberShadow]

(see comment)

[CyberShadow]

Sorry, I got d_namlen and d_reclen confused.

I'm pretty sure d_namlen can be safely used to determine the length of the file name, but what about d_reclen? I don't think that's the general case, right?

@edi33416 Did you test that this change works on Solaris? I don't think we have any CIs actually testing it on that system.

[edi33416]

Sorry, I got d_namlen and d_reclen confused.

I'm pretty sure d_namlen can be safely used to determine the length of the file name, but what about d_reclen? I don't think that's the general case, right?

Yes, you are right. I can use d_namlen when it's available to determine the length (it gives the len without the null terminating character).

On Solaris, you don't have d_namlen, but you can use d_reclen to determine the length of d_name.
The best explanation I could find is in this StackOverflow answer.

The correct way to find the length, I believe, is like the _D_ALLOC_NAMLEN macro

• if you have d_namlen, use it
• else if you have d_reclen, use that
• else use strlen

@edi33416 Did you test that this change works on Solaris? I don't think we have any CIs actually testing it on that system.

@CyberShadow I didn't. I don't have a Solaris VM, and was counting on the CIs. I'll set one up

[CyberShadow]

• else if you have d_reclen, use that

I don't think that's cross-platform / POSIX-correct... We can use it as an optimization for platforms where we know it aligns with the end of the filename, or when the _D_ALLOC_NAMLEN macro is available. It doesn't look like the links you posted also have any indication that reclen can be reliably used to find the length of the file name, as there could be padding at the end.

I don't have a Solaris VM, and was counting on the CIs. I'll set one up

I don't think that's necessary as long as we stick to simple code that sticks to what's in POSIX.

Applied my own suggestions

[edi33416]

• else if you have d_reclen, use that

I don't think that's cross-platform / POSIX-correct... We can use it as an optimization for platforms where we know it aligns with the end of the filename, or when the _D_ALLOC_NAMLEN macro is available. It doesn't look like the links you posted also have any indication that reclen can be reliably used to find the length of the file name, as there could be padding at the end.

Yes, it does seem like an optimization. I misread the ifdef [..] else

I don't have a Solaris VM, and was counting on the CIs. I'll set one up

I don't think that's necessary as long as we stick to simple code that sticks to what's in POSIX.

LGTM

[edi33416]

@thewilsonator the failing buildkite seems unrelated. Any ideas regarding why buildkite is failing?

[thewilsonator]

I'm pretty sure that got fixed recently, this PR predates that I think.