[PATCH] D94424: [clangd] Make AST-based signals available to runWithPreamble.

[Utkarsh Saxena via Phabricator via cfe-commits]

usaxena95 added inline comments.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:841
 
+void ASTWorker::updateASTSignals(ParsedAST &AST) {
+  ASTSignals Signals;
----------------
sammccall wrote:
> This implementation doesn't belong in TUScheduler, which is all about managing lifecycles and threading, rather than inspecting ASTs.
> 
> Best suggestion I have is to create a separate header ASTSignals.h, which defines the ASTSignals struct and a `static ASTSignals ASTSignals::derive(const ParsedAST&)` function to extract them.
> 
> (It's pretty awkward to even directly depend on it, since this feels more of a "feature" like go-to-definition that's independent of to TUScheduler and injected by ClangdServer, rather than core things like ParsedAST that are layered below TUScheduler. But fundamentally we can't avoid this without somehow obfuscating the data structure we're storing).
Moved struct and calculation to a separate file ASTSignals.h/cpp


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:851
+            Signals.Symbols[ID] += 1;
+          // FIXME: Do not count the primary namespace as a related namespace.
+          // Eg. clang::clangd:: for this file.
----------------
sammccall wrote:
> why not?
My mental model suggested these must be namespaces from outside the file which are relevant to this file.
I suppose we can take care of this while using this (in CC signals calculation) and let these represent all namespaces including the primary one.

On a second thought: If we do not explicitly exclude primary namespace, the results from primary namespace will be boosted much more than the results from index which might be acceptable too.



================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:853
+          // Eg. clang::clangd:: for this file.
+          if (const auto *NSD = dyn_cast<NamespaceDecl>(ND->getDeclContext()))
+            if (!ND->isInAnonymousNamespace()) {
----------------
sammccall wrote:
> as mentioned above, you may want to do this only if the per-symbol-count was incremented from 0 to 1.
> You'd have to ignore namespaces for symbols with no symbolID, but that doesn't matter.
Ignored NS without a valid SymbolID.
Counted distinct number of Symbols used per namespace.


================
Comment at: clang-tools-extra/clangd/TUScheduler.cpp:854
+          if (const auto *NSD = dyn_cast<NamespaceDecl>(ND->getDeclContext()))
+            if (!ND->isInAnonymousNamespace()) {
+              std::string NS;
----------------
sammccall wrote:
> why not NSD->isAnonymous()?
> 
> (and generally prefer early continue rather than indenting the whole loop body)
I guess `isAnonymous` would cover most cases. I thought of excluding named NS in an anonymous NS but those are super rare I suppose and including those is not harmful too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94424