This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Add language metrics for recovery AST usage.
ClosedPublic

Authored by hokein on Nov 26 2020, 12:41 AM.

Diff Detail

Event Timeline

hokein created this revision.Nov 26 2020, 12:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 26 2020, 12:41 AM
hokein requested review of this revision.Nov 26 2020, 12:41 AM
kadircet added inline comments.Nov 26 2020, 1:10 AM
clang-tools-extra/clangd/Selection.cpp
51

nit: it might be nice to bail out early if no tracer is attached with trace::enabled

njames93 added inline comments.
clang-tools-extra/clangd/Selection.cpp
40

Nit: any reason to use std::string here instead StringRef.

hokein updated this revision to Diff 307852.Nov 26 2020, 6:17 AM

address comment.

hokein marked 2 inline comments as done.Nov 26 2020, 6:20 AM
hokein added inline comments.
clang-tools-extra/clangd/Selection.cpp
40

changed to const char *

51

good catch, thanks! (I didn't know there is such a thing).

sammccall added inline comments.Dec 4 2020, 3:04 AM
clang-tools-extra/clangd/Selection.cpp
41

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.

63

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?)

hokein updated this revision to Diff 309817.Dec 7 2020, 12:04 AM
hokein marked 2 inline comments as done.

address review comments.

clang-tools-extra/clangd/Selection.cpp
41

sounds good to me.

63

yeah, this is a good idea, didn't realize that we could use the label, thanks!

sammccall accepted this revision.Dec 7 2020, 12:10 AM
sammccall added inline comments.
clang-tools-extra/clangd/Selection.cpp
54

This classifies objc++ as c++.
Not necessarily a problem, up to you

This revision is now accepted and ready to land.Dec 7 2020, 12:10 AM
hokein added inline comments.Dec 7 2020, 1:39 AM
clang-tools-extra/clangd/Selection.cpp
54

yeah, this is my intention as we don't really care about C++ or ObjC++.

This revision was automatically updated to reflect the committed changes.