Make Loc.toChars pure and nothrow

[jacob-carlborg]

No description provided.

[dlang-bot]

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

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

[thewilsonator]

Loc is a very important struct w.r.t memory consumed by the compiler.

Since showColumns is only used in that method it would be better to make it a default parameter of toChars().

[jacob-carlborg]

Walter doesn't like default parameters as far as I know. But I can try to make it a required parameter.

[thewilsonator]

As long as its not taking up more memory than is necessary I really don't care. A required parameter is fine, might make the diff a bit bigger. @WalterBright any preferences?

[jacob-carlborg]

Using a parameter with a default value in toChars now.

[WalterBright]

Hiding the use of a global behind a default value is not a good practice, it's faking a good practice. Either make it explicit or not pure.

[andralex]

Been thinking of this for a while, seems like a good step forward.

Before toChars():

• Not pure
• Use of globals entirely opaque (why is it not pure? what global(s) does it need?)
• Impossible to call from a pure context

After toChars(bool showColumns = global.params.showColumns):

• Signature shows what global is involved - very informative!
• Pure caller code can call it by passing a bool in
• Better yet, that bool can be threaded in from the call chain all the way to the top level, where only one function is impure and passes global.params.showColumns down.

This is akin to Haskell's way of carrying monads around, and (less so) Scala's implicits. I think we're in good company.

I'm in favor of this idiom.

[jacob-carlborg]

Yes, thank you Andrei. I tried making it a required parameter but that resulted in too many change so I gave up. I was both tiresome to do all those changes and the resulting diff would have been too large.

With this approach it's now possible continue modifying the code base to explicitly pass in the showColumns argument one call at the time, instead of hundreds at once.

[jacob-carlborg]

Another approach would be to instead of printing the error immediately, collect them a print them later. Then the printing function could implement what Loc.toChars is doing today and take global.params.showColumns into consideration as well. Here's an example of where I implemented this:

dmd/src/dmd/errors.d

Lines 161 to 170 in e42f83e

	abstract class Diagnostic
	{
	/// The location of where the diagnostic occurred.
	const Loc loc;
	/// The severity of the diagnostic.
	const Severity severity;
	/// The message.
	abstract string message() const nothrow;

Part of #9480.