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

[Haojian Wu via Phabricator via cfe-commits]

hokein accepted this revision.
hokein added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang-tools-extra/clangd/InlayHints.cpp:51
   bool VisitCXXConstructExpr(CXXConstructExpr *E) {
+    if (!Cfg.InlayHints.DeducedTypes)
+      return true;
----------------
sammccall wrote:
> 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 :-\
Thanks for digging into this.

> ~5ms for current impl (prefilter) with all categories disabled. (On SemaCodeComplete.cpp, just eyeballing traces)

Yeah, we should have this prefilter implementation when all categories are disabled.


> Debug build on my laptop is ~170ms for all hints, ~160ms for postfilter with all categories disabled,

The data doesn't seem reasonable, I think the total cost comes from two main sources (AST traversal cost + assembling JSON object cost),
I would expect the `allhints` and `postfilter` have the similar AST traversal cost, but `postfilter` should have a near zero cost of assembling JSON objectcs (as it returns an empty vector), so the AST traversal cost is ~160ms.

> Also I should work out what we're spending 170ms on... EDIT: it turns out it's just assembling JSON objects :-\

And based on this, it turns out the cost of `all-hints` approach should be larger than 170ms, which should be ~300ms (160ms for AST traversal + 170ms for assembling JSON objects).


Anyway, the current implementation looks good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116713