[PATCH] D79701: [clangd] Add metrics for selection tree and recovery expressions.

[Sam McCall via Phabricator via cfe-commits]

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