Page MenuHomePhabricator

[clangd] Deduplicate inlay hints
ClosedPublic

Authored by nridge on Sep 19 2021, 11:28 PM.

Details

Summary

Duplicates can sometimes appear due to e.g. explicit template
instantiations

Diff Detail

Event Timeline

nridge created this revision.Sep 19 2021, 11:28 PM
nridge requested review of this revision.Sep 19 2021, 11:28 PM

I will note that I considered handling this in a way specific to explicit template instantiations.

However, while I do not have a reduced testcase for it, I've also run into duplicate hints in the context of macros.

So, it seemed easier to deduplicate in general; there is no justification for showing the same hint multiple times in the same location.

I am not aware of how the intersecting hints are handled in the implementation on the client side nor in the proposal today. After this patch we might still produce them if for whatever reason there are different kinds of hints for the same range. Is this OK?

clang-tools-extra/clangd/InlayHints.cpp
372

nit: You can just do llvm::sort(Results)

I am not aware of how the intersecting hints are handled in the implementation on the client side nor in the proposal today. After this patch we might still produce them if for whatever reason there are different kinds of hints for the same range. Is this OK?

The behaviour with the duplicated hints in vscode was that they appeared one after the other, e.g. : int : int : int. So, wrong but not catastrophic.

I don't think we're likely to get a parameter hint and a type hint at the same location, but if you're concerned, I could revise the patch to ignore the hint kind during the comparison.

nridge updated this revision to Diff 373782.Sep 20 2021, 11:02 PM

address nit

kadircet accepted this revision.Sep 21 2021, 12:14 AM

LGTM, thanks!

I don't think we're likely to get a parameter hint and a type hint at the same location, but if you're concerned, I could revise the patch to ignore the hint kind during the comparison.

That's hard call to make, there are probably good arguments for:

  • keeping them all
  • keeping only one
  • dropping them all.

I was just worried about breaking clients. We should probably account for this in the spec, as by probably forbidding any intersecting hints completely for a better UX (as discussed with @sammccall offline).

This revision is now accepted and ready to land.Sep 21 2021, 12:14 AM
This revision was automatically updated to reflect the committed changes.