sammccall added inline comments.
================ Comment at: clangd/Quality.cpp:70 + return SymbolRelevanceSignals::ClassScope; + if (D.getLinkageInternal() < ExternalLinkage) + return SymbolRelevanceSignals::FileScope; ---------------- ioeric wrote: > I wonder if this comparison is a common practice. Any reason not to use `!=`? The orderedness in linkage is used e.g. in `isExternallyVisible()`. I prefer `<` because while with this particular threshold value `!=` would be equivalent, I'm not actually particularly sure it's the right threshold (vs e.g. `ModuleLinkage`), and with any other threshold `<` would be required. ================ Comment at: clangd/Quality.cpp:71 + if (D.getLinkageInternal() < ExternalLinkage) + return SymbolRelevanceSignals::FileScope; + return SymbolRelevanceSignals::GlobalScope; ---------------- ioeric wrote: > Does `FileScope` indicate that `D` is defined in the current *translation > unit*? If so, I think this is a good signal (boosting symbols in the same > TU), but something like `TUScope` might be a bit clearer? File rather than TU is intended here. `AccessibleScope` is only an approximate concept (it doesn't need to be precise as it's just a scoring signal). The idea is "this is a symbol likely to be only used in the same file that declares it", and this is true of e.g. variables in anonymous namespaces, despite the fact that you can technically put them in headers and reference them in the main file. Added a comment to AccessibleScope to clarify this approximate nature. ================ Comment at: clangd/Quality.cpp:86 + if (SemaCCResult.Kind == CodeCompletionResult::RK_Declaration) + Scope = std::min(Scope, ComputeScope(*SemaCCResult.Declaration)); } ---------------- ioeric wrote: > nit: does this assume that "smaller" scopes are better? If so, it might make > sense to document. There's no connection with "better" here, just that items are assumed to have maximum scope (global) until proven otherwise. There's no intent here (or with other signals) for merging conflicting results from different sources to do anything clever - it's fine to choose one arbitrarily. ================ Comment at: unittests/clangd/QualityTests.cpp:1 //===-- SourceCodeTests.cpp ------------------------------------*- C++ -*-===// // ---------------- ioeric wrote: > Could you also add a test case for code completion? The code completion scoring is tested in SymbolRelevanceSignalsSanity: file scope is boosted compared to default when the query is code-complete, but not when it's generic. What kind of test do you mean? Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D47762 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
