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