sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:39
+void recordMetrics(const SelectionTree &S) {
+ static constexpr trace::Metric Selection("selection",
trace::Metric::Counter);
----------------
this deserves a comment about what we're tracking and why.
Like "measure the fraction of selections that were enabled by recovery AST"
================
Comment at: clang-tools-extra/clangd/Selection.cpp:40
+void recordMetrics(const SelectionTree &S) {
+ static constexpr trace::Metric Selection("selection",
trace::Metric::Counter);
+ static constexpr trace::Metric SelectionRecoveryAST("selection_recovery_ast",
----------------
just realized a slightly more elegant(?) way to measure this:
```
Metric SelectionUsedRecovery("selection_recovery", Distribution);
...
for (...) {
if (...) {
SelectionUsedRecovery.record(1); // used
return;
}
}
SelectionUsedRecovery.record(0); // not used
```
Now the count is the total number of selections, and the average is the
fraction that used recovery.
================
Comment at: clang-tools-extra/clangd/Selection.cpp:48
+ if (N->ASTNode.get<RecoveryExpr>()) {
+ // FIXME: tracing the type through the label?
+ SelectionRecoveryAST.record(1);
----------------
FWIW, my original RecoveryExpr proposal included a StmtClass for the type of
expr that was "supposed" to be here. (e.g. CallExpr for a failed overload
resolution). It was dropped to keep the scope minimal.
I'm not sure if that exact idea generalizes to all the places that RecoveryExpr
is used, but it would be great to somehow keep track of the semantic context
where recovery was used. And I think it would make a great label here. Maybe
this would be better as a dedicated enum, or just a string literal (e.g.
"variable initializer").
Anyway, an idea for another time...
================
Comment at: clang-tools-extra/clangd/Selection.cpp:48
+ if (N->ASTNode.get<RecoveryExpr>()) {
+ // FIXME: tracing the type through the label?
+ SelectionRecoveryAST.record(1);
----------------
sammccall wrote:
> FWIW, my original RecoveryExpr proposal included a StmtClass for the type of
> expr that was "supposed" to be here. (e.g. CallExpr for a failed overload
> resolution). It was dropped to keep the scope minimal.
>
> I'm not sure if that exact idea generalizes to all the places that
> RecoveryExpr is used, but it would be great to somehow keep track of the
> semantic context where recovery was used. And I think it would make a great
> label here. Maybe this would be better as a dedicated enum, or just a string
> literal (e.g. "variable initializer").
>
> Anyway, an idea for another time...
not clear quite what you mean by this - the *actual* type isn't a suitable
label, as it's a large unbounded set.
The presence/absense might be interesting I guess, but I'm not sure about it:
it's unclear what significance it has if the containing expression's type is
know, if we're selecting something inside it. (More significant if the selected
expression's type is known/unknown because a contained RecoveryExpr's type is
known/unknown, but we can't track that)
I'm not sure this is a FIXME, it seems most likely we decide not to fix it.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D79701/new/
https://reviews.llvm.org/D79701
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
