codespan icon indicating copy to clipboard operation
codespan copied to clipboard

move locus calculation to separate function

Open Johann150 opened this issue 4 years ago • 3 comments

This moves the locus calculation from RichDiagnostic::render to Diagnostic::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 the FileId, the FileId must now implement Ord instead of just PartialEq.

    This was the simplest and least overhead accumulating way to enable deduplication by file. Alternatively we could derive another arbirary ordering of the FileIds, e.g. by indices from a deduplicated Vec.

    I personally think this change is justified because

    • many library users make use of SimpleFiles, which is compatible and
    • any sensible candidates for FileId that I can think of and implement PartialEq also implement Ord.
  • [ ] Should this new function be added to the public API? I did not yet implement it this way, but it was suggested in #333. This would go towards better enabling "alternative" renderers.

Johann150 avatar Aug 12 '21 19:08 Johann150

Please note the CI is failing because of dependencies not being compatible with the oldest tested against Rust version (1.40.0).

Johann150 avatar Aug 12 '21 19:08 Johann150

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!

brendanzab avatar Aug 22 '21 00:08 brendanzab

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.

Johann150 avatar Aug 22 '21 14:08 Johann150