move locus calculation to separate function #334
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This moves the locus calculation from
RichDiagnostic::render
toDiagnostic::locuses
. Some questions to answer:Is the implementation impact acceptable?
The way I implemented this has some impacts on the existing API though: Because this way uses a
BTreeMap
keyed with theFileId
, theFileId
must now implementOrd
instead of justPartialEq
.This was the simplest and least overhead accumulating way to enable deduplication by file. Alternatively we could derive another arbirary ordering of the
FileId
s, e.g. by indices from a deduplicated Vec.I personally think this change is justified because
SimpleFiles
, which is compatible andFileId
that I can think of and implementPartialEq
also implementOrd
.Should this new function be added to the public API?
I did not yet implement it this way, but it was suggested in Accessor for the location of a diagnostic #333. This would go towards better enabling "alternative" renderers.