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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
