Skip to content

Conversation

gorsing
Copy link
Contributor

@gorsing gorsing commented Jul 8, 2025

This change refactors splitPath and appendSplitPath to use static function pointer callbacks with an explicit context pointer (void*), instead of D closures (delegates).

  • Replaces all delegate-based callbacks with function pointer + context.
  • Avoids capturing variables with destructors or complex lifetimes in closures.
  • Improves code safety and compatibility with C-style APIs.
  • Updates all usages of splitPath and appendSplitPath across the codebase and tests.
  • No changes to external behavior or API (except callback signature).

This prepares the codebase for future improvements, better static analysis, and eliminates risks associated with closure captures in system code.

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @gorsing! 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

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 run digger -- build "master + dmd#21516"

Copy link
Contributor

@thewilsonator thewilsonator left a comment

Choose a reason for hiding this comment

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

This prepares the codebase for future improvements, better static analysis, and eliminates risks associated with closure captures in system code.

Why is this required for that? In isolation, this change does not look to be desirable.

@gorsing
Copy link
Contributor Author

gorsing commented Jul 9, 2025

Great question, and I appreciate the opportunity to clarify the motivation behind this change.

"Why do this? On its own, this change doesn't seem desirable."

You're absolutely right — moving from delegates to function pointers with an explicit context may seem like a step back in terms of ergonomics and readability. However, this change is less about immediate benefits and more about reinforcing long-term safety and clarity, especially in system-critical parts of the codebase like root/.

Here's why this approach was chosen, and how it aligns with broader goals:

1. Prevents Unsafe or Inconsistent Closure Captures

As outlined in [Issue #21497], closures that capture variables with destructors can lead to subtle and dangerous bugs — including double destruction and use-after-free. These issues often occur silently, when closures unintentionally capture more than intended.
By switching to an explicit (function pointer + void* context) model, we eliminate the risk of such accidental captures, especially for types with complex or non-trivial lifetimes.

2. Enables Better Static Analysis and Code Auditing

Closures hide what is captured, which complicates both compiler reasoning and human review. Explicit context passing makes ownership and data flow more transparent, supporting safer refactoring, improved diagnostics, and better tooling support.

3. Aligns with C-Compatible System-Level Design

Modules in root/ are designed to be minimal, predictable, and C-style. Using function pointers with an explicit context fits this philosophy better than D closures, which may introduce hidden heap allocations or GC roots — often unintentionally.

4. Proactively Aligns with Potential Language Evolution

PR #21514 proposes stricter rules regarding closure captures involving destructors. While this direction is still under discussion within the community, adopting a more explicit and controlled approach at this stage helps ensure forward compatibility with possible future language semantics. This change avoids potentially fragile patterns and decouples the current refactoring from future adjustments, making any transition smoother and more predictable.

That said, this is merely a suggestion based on my personal perspective, and I fully understand that the community may ultimately decide on a different path.

5. Improves Debugging and Maintenance Experience

Delegate-based closures introduce implicit memory management and runtime complexity that can obscure stack traces or complicate debugging. Function pointers with explicit context are simpler, more transparent, and easier to reason about in low-level scenarios.


So, while the change might appear more verbose at the call site, it's a deliberate tradeoff: clarity, safety, and maintainability over brevity. In areas of the codebase where long-term stability is paramount, this approach helps mitigate risk and reduce technical debt.

Of course, I’m open to alternative suggestions if there’s a cleaner or more ergonomic solution that achieves the same safety guarantees — feedback is always welcome.

@gorsing gorsing requested a review from thewilsonator July 9, 2025 09:02
@gorsing
Copy link
Contributor Author

gorsing commented Jul 9, 2025

@thewilsonator What are your arguments?

@rikkimax
Copy link
Contributor

rikkimax commented Jul 9, 2025

After review by myself and Nic, we have concluded that this is not an appropriate solution.

It is a very C centric solution, by using the void* pointer that is explicitly passed in, this is not type safe.
A type-safe solution would involve using D's metaprogramming capabilities with templates.

However, this is not required, if you apply scope to the parameter, it would have removed the closure in the case presented.
The concern we have with this PR, is a lack of understanding of the D programming language in the context of the change in question.
It would require follow-up work, which may not be required if this PR is not completed.

You are more than welcome to join us on Discord if you haven't already.

@gorsing
Copy link
Contributor Author

gorsing commented Jul 9, 2025

Thank you for the detailed feedback. I understand your concern regarding type-safety and the C-style approach.
I’ll review how scope could be applied in this context instead, and will study how this kind of problem is usually solved idiomatically in D.
I appreciate the invitation to the Discord — I’ll join to continue the discussion and learn. Thanks again!

@gorsing
Copy link
Contributor Author

gorsing commented Jul 9, 2025

As a next step, I’d like to explore a cleaner and more idiomatic approach using scope and possibly templates to maintain type safety.

I’m going to prototype an alternative version based on your suggestions. Once I have something solid, would it make sense to open a new PR — or would you prefer continued discussion here first?

Thanks again for the guidance — I’m looking forward to improving the fix the right way.

@rikkimax
Copy link
Contributor

rikkimax commented Jul 9, 2025

I have replied to your other PR, explaining that the issues you are seeing stem from lifetimes not being respected.
I suspect that you will not be able to come up with a solution that only errors for bad behaviour, but is ok on marginal code.

You are welcome to give it a go in another PR, such exploratory work is always good for learning new things. But please don't use LLM's to generate text in response to us.

@gorsing
Copy link
Contributor Author

gorsing commented Jul 9, 2025

I really appreciate your patience and guidance.

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

Successfully merging this pull request may close these issues.

4 participants