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
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
