[PATCH] D116713: [clangd] Support configuration of inlay hints.

[Sam McCall via Phabricator via cfe-commits]

sammccall added inline comments.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+    if (!Cfg.InlayHints.DeducedTypes)
+      return true;
----------------
hokein wrote:
> this should be `!Cfg.InlayHints.ParameterNames`.
> 
> What do you think the idea of moving guards deeper (`processCall` and `addTypeHint`)? The code seems clearer and we don't have to add them in all Visit* implementation,  this means that we pay cost of running some necessary code, but I think it is trivial and probably worthy. 
I agree where to put the checks is an annoying question (and worth minimizing, since I've managed to get two wrong already).
I do think there needs to be a pattern so we don't accidentally skip checks.

- Easest is to postfilter (check in addInlayHint). That *does* hurt performance. Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled, ~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)
- Checking in VisitXXX methods (as done here) is a very regular pattern. Downside is you need many checks, and you can forget/break one
- Checks in helpers so that one is always hit (as you propose) touches fewer places but it's ad-hoc. I'm afraid of getting it wrong as the impl is modified (missing checks, doing too much work first, etc).
- Splitting RAV per category is an interesting option. Checking is very elegant, nicer code structure, can trace per-category latency, disabled categories can't crash... The downside is extra overhead, but this must be <5ms in the performance above. We can still choose to bundle simple categories together...

I think I'll do as you propose for now, improve the tests, and refactor to try to make it feel less ad-hoc later.
Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116713