Open Johann150 opened 3 years ago
Please note the CI is failing because of dependencies not being compatible with the oldest tested against Rust version (1.40.0).
I'm wondering if there is as way to do this without needing to allocate an intermediate BTreeMap
for this? 🤔 That's the one thing that would give me pause I guess.
We might also be able to solve this better if we had some sort of intermediate 'resolved diagnostic' - something I'm hoping we can get to at some stage!
The reason for using a BTreeMap was this comment https://github.com/brendanzab/codespan/blob/629d35a4c9f30236a2adbc5773c756d23d1775d7/codespan-reporting/src/term/views.rs#L98-L100
I would not call it an "intermediate BTreeMap
" because its used afterwards, like the vector the above comment is referencing.
I don't see a way without allocating some data structure similar to this. We could do with a Vec
but that does not come with the deduplication built in, and no advantages that I am aware of.
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
.