[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

Tue Feb 7 01:06:30 PST 2023

kadircet marked an inline comment as done.
kadircet added a comment.

In D143096#4099662 <https://reviews.llvm.org/D143096#4099662>, @sammccall wrote:

> It looks like this fixes up the location only of diagnostics attached to particular directives (`#include`) based on some "deep" idea about the content of the directive (the spelled header name).
>
> Some shortcomings:
>
> - This misses diagnostics attached to other directives / continued lines in macro definitions / etc
> - it allows diagnostics to be translated across lines even if the directive spelling changed (in which case the ranges are incorrect since only line numbers are updated.)
> - it requires modelling the directive content in some way that needs to be extended if we find another place that diagnostics can be attached
>
> We discussed the idea of attaching to the text of the line, which seems pretty generic. Needs some handling of duplicate line content but seems like something pretty naive (closest line number with matching content?) would work well, be just as simple and more generic. Any reason this alternative was rejected?

agreed this is better, also somewhat more elegant on the implementation side. switching to a line-content based translation, while keeping the logic around limiting diagnostics to single lines. as multiple lines seem more wonky and unclear how common they're.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D143096/new/

https://reviews.llvm.org/D143096