rewrite writeCharLiteral() to use sinks

[WalterBright]

tokens.d suffers from global variables and memory leaks. This impairs using D as an LSP. This PR starts a bottom_up approach to fixing this by using output sinks.

Lambdas would be cleaner, but I don't think the bootstrap compiler supports the lambda syntax yet.

[dlang-bot]

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

[dkorpel]

Hold on, OutBuffer is a nice abstraction, and a common type across dmd. Replacing it with a crappier void delegate(char) interface is not helping anything, it only duplicates OutBuffer internals on every former call site.

I don't see any globals being removed or leaks being plugged this way, all I see is:

• ancient C-style snprintf into a static char[n] = void buffer (need to manually calculate upper bound)
• more possible null segfaults (delegate can be null, ref OutBuffer can't)
• less safe interfaces
• more goto
• more loops
• more lines of code

[WalterBright]

@dkorpel search tokens.d for "sometimes leaking memory" and "__gshared".

[dkorpel]

The solution is to give Token.toString a void toString(ref OutBuffer buf) interface, and make the old const(char)[] toString() forward to that. The latter interface is always going to be re-using a global buffer or leaking, using a callback sink in the internals doesn't change that.

[WalterBright]

I would have agreed with you a while ago, but I like the idea of "no memory allocations" better. It fits in cleanly with doing multiple instances, multithreading, etc. It's easier to write unittests for them. It's easier to understand the code because there are fewer dependencies. There's no need to manage memory. I also suspect the sink technique results in faster code.

The latter interface is always going to be re-using a global buffer or leaking, using a callback sink in the internals doesn't change that.

I aim to fix that. In any case, this scheme means fewer leaks. And I cannot fix the upper level leaks without fixing the lower level ones.

[thewilsonator]

if you want no allocations this needs to be a scope delegate

[WalterBright]

The idea is the sink is an abstraction, and whether it allocates or not and how it allocates is up to the caller, not the callee. The idea is to push the allocation decisions to the top level of the code hierarchy, rather than in the leaves.

[thewilsonator]

yes, but this delegate is not saved anywhere. If the delegate passed to this function closes over any variables, then this will allocate and that is not up the the callee code. Putting scope on the delegate does not preclude the callee from doing anything.

[Herringway]

Don't forget that a non-scope delegate parameter requires allocating a closure, as its lifetime may be longer than the caller and callee. While that isn't a leak, it's likely unwanted and certainly unnecessary here.

[dkorpel]

Like you say, "D is plastic". You shouldn't have to convert hundreds of call sites to commit to this specific idea, especially if this hasn't been confirmed to work yet. What is this going to look like at the 'upper levels'? Invariably you'll hit a case like this at some point:

error("%s", token.toChars);

Then what, are we giving error a void delegate(char)[] sink... interface and convert another 1000+ call sites? If this approach turns out to be a bust, are we reverting all that code churn again? As it stands, this PR doesn't fix any leaks, and I don't see how it's going to.

[WalterBright]

this PR doesn't fix any leaks

It removes the memory allocations, which come from the buf.write... calls, pushing the responsibility up the call stack. writeCharLiteral() is called in two places. The first is on line 1074, and then the memory leaks on line 1079. The next PR would be to push that allocation up the stack, eliminating those leaks.

The second call is on line 1229, which will get a refactor in the next PR to also use a sink.

Regarding the proliferation of sink declarations, those will go away as the sinks will be passed as a parameter and will not need to be redefined in every function. Only the root will need to define the sink.

You are correct in that the top level toChars() is fundamentally a memory leak. But, the current design means the implementation of toChars() is heaping on a bunch of additional memory leaks, which I am intent on eliminating. The top level toChars() relies on toString(), which also leaks. I looked into the calls of toString(), and they can be refactored to eliminate the leak.

That leaves toChars(). Even if we don't fix that, we still wind up eliminating a nest of other leaks. Fixing it will require a bit of rethinking of how it should work.

The bottom line is having __gshared variables and memory leaks is going to cause problems with LSP.

Like you say, "D is plastic". You shouldn't have to convert hundreds of call sites to commit to this specific idea,

The original implementation was in C, and this is "C-style" code, not D-style. Changing the abstraction to a "sink" is not very practical in C, as C doesn't have delegates. The sink is completely generic, as it pushes all the dependencies to the caller, not the low level leaves. Old code of mine does allocations in the leaves. I've learned the hard way that this always leads to difficulties.

Please indulge me with this effort to make the lexer more tractable. I think you'll like the result. (I've advocated this approach in my slide presentations!)

The 1000 lines to fix memory leaks is an acceptable price, and I am not foisting it off on anyone but myself.

[dkorpel]

The bottom line is having __gshared variables and memory leaks is going to cause problems with LSP.

This PR doesn't change anything about that. Whatever follow up you were planning to do to actually fix those issues, you can also do without turning a relatively safe interface like this:

buf.printf("\\x%02x", d);

Into a brittle, bug prone interface like this:

char[2 + 8 + 1] buf = void;

n = snprintf(buf.ptr, n, "\\x%02x", d);

foreach (chr; buf[0 .. n])
    sink(chr);

[dkorpel]

Please indulge me with this effort to make the lexer more tractable. I think you'll like the result. (I've advocated this approach in my slide presentations!)

The 1000 lines to fix memory leaks is an acceptable price, and I am not foisting it off on anyone but myself.

I've been on this rodeo many a times. How it usually plays out:

• The refactor turns out to be more tedious work than expected
• The pay off is too low, work gets abandoned in favor of more pressing issues
• The code lingers in a half-refactored state
• An easier, more pragmatic solution gets implemented instead

In this case, the pragmatic solution is probably making OutBuffer use the GC or an arena allocator instead of hard-coded malloc.

[WalterBright]

You have a good point:

buf.printf("\\x%02x", d);

Yes, that is a nice safe interface, and the snprintf is rather klunky. But you gave me a good idea! The buf version is great because I spent the time in OutBuffer making it slick, it is not an inherent property of buf. What I need to do is create:

sink.printf("\\x%02x", d);

which will put it on par with buf.

Also, I have consolidated this PR and #23020 into #23025, which is a completed version of the refactoring, so I'll be closing this and #23020. Please continue this discussion there.