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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
