[PATCH] D92157: [clangd] Add language metrics for recovery AST usage.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/Selection.cpp:41
+const char *getLanguage(const clang::LangOptions &Lang) {
+  if (Lang.C99 || Lang.C11 || Lang.C17 || Lang.C2x)
+    return "C";
----------------
sammccall wrote:
> Testing for specific C versions seems a bit weird to me - what if we're in C89? 
> I'm not sure what the intention is - if the idea is to exclude extensions like openmp I'm not sure this actually does that.
> And by checking it before ObjC I think we're misclassifying `clang -std=c99 foo.m`
> 
> I'd suggest checking (optionally objc++), objc, c++, and calling everything else C. If you really want to avoid particular dialects, probably best to name them specifically.
sounds good to me. 


================
Comment at: clang-tools-extra/clangd/Selection.cpp:65
       RecoveryType.record(RE->isTypeDependent() ? 0 : 1);
+      RecoveryAvailable.record(1, getLanguage(AST.getLangOpts()));
       return;
----------------
sammccall wrote:
> I'm not clear what this is trying to measure - why isn't this the same metric as SelectionUsedRecovery (just adding a field to that one?)
yeah, this is a good idea, didn't realize that we could use the label, thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92157