SourceLocation: refactor user-facing part of SourceLocation out and allow it to be computed on demand.

[nkcsgexi]

Always computing the user-facing part of source location (line/column) can be
inefficient and unnecessary. This patch refactors the user-facing
part of SourceLocation to a separate data structure and allows it to
be computed on demand.

[nkcsgexi]

@swift-ci please test

[akyrtzi]

Instead of doing this as separate mutating step, how about have the initializer accept an optional converter: public init(offset: Int, converter: SourceLocationConverter? = nil) {

If the caller passes the converter it means it wants the computation of line/column to occur, otherwise the SourceLocation will only contain the offset.

[nkcsgexi]

We have two options here for parser side diagnostics: (1) the client provides a bool indicating whether they want the line/column computed; or (2) the diagnostics are always without line/column and it's up to the client to use computeLineColumn to get them if needed.

I'm leaning towards (2) because we can avoid adding another argument to parse() and client can have more control over diagnostics, for instance, filtering them before displaying.

[akyrtzi]

The downside is that it increases the mental overhead for the client for handling diagnostics. The client needs to keep track of multiple things:

• Keep track of whether a diagnostic source creates diagnostics with line/columns or not. The parser does not but another source of diagnostics that the client is using may be creating such diagnostics.
• Figure out where the line/column conversion should occur and keep track of it. What if there are multiple places where it needs the line/column of the diagnostic, should it be doing the line/column conversion at multiple places, where it needs it, or should it be doing it once earlier, and how to keep track that.

Ideally the client should only worry about one thing, whether the source is creating diagnostics with line/columns or not.

However, I'm also not a fan of passing a boolean to the parser, so how about this alternative:

• The parser just receives a DiagnosticConsumer, and always feeds it "offset" diagnostics
• We add a LineCalculatingDiagnosticConsumer to the API which is a concrete implementation of DiagnosticConsumer that receives diagnostics, converts the diagnostics to have line/column, and feeds them to another DiagnosticConsumer. Something like this:

myDiagConsumer = MyDiagnosticConsumer() // the client's consumer
calcDiagConsumer = LineCalculatingDiagnosticConsumer(myDiagConsumer, sourceLocConverter) // converts offset to line/column and passes such diagnostics to `myDiagConsumer`
SyntaxParser.parser(source: source, diagnosticConsumer: calcDiagConsumer) // pass a diagnostic consumer to parser.

If the client doesn't need to have line/column in the parser diagnostics then just remove the second line.

[harlanhaskins]

I think not calculating lines and columns is the surprising behavior, not the performance hit of calculating them. I still firmly believe calculating lines and columns should be the default.

There’s no extra mental overhead for the common case, unless they’ve opted in to skipping line/column calculation.

[akyrtzi]

How about it is the DiagnosticConsumer that indicates it wants diagnostics with line/columns via a property in the protocol ? It will be true by default. If you don't need line/columns set it to return false in your consumer implementation.

This bundles together processing of diagnostics and determination of whether they will contain line/columns.

[harlanhaskins]

That one sounds like a winner to me!

[nkcsgexi]

Done with the change for the initializer of SourceLocation. Will open another PR for DiagnosticConsumer.

[nkcsgexi]

@swift-ci please test

[swift-ci]

Build failed
Swift Test OS X Platform
Git Sha - a72fea3