ioeric accepted this revision. ioeric added a comment. This revision is now accepted and ready to land.
lgtm ================ Comment at: clangd/Quality.cpp:70 + return SymbolRelevanceSignals::ClassScope; + if (D.getLinkageInternal() < ExternalLinkage) + return SymbolRelevanceSignals::FileScope; ---------------- sammccall wrote: > 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. sounds good. this would probably deserve a comment though as it's a little confusing... ================ Comment at: clangd/Quality.cpp:71 + if (D.getLinkageInternal() < ExternalLinkage) + return SymbolRelevanceSignals::FileScope; + return SymbolRelevanceSignals::GlobalScope; ---------------- sammccall wrote: > 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. Cool. Thanks for the explanation! ================ Comment at: unittests/clangd/QualityTests.cpp:1 //===-- SourceCodeTests.cpp ------------------------------------*- C++ -*-===// // ---------------- sammccall wrote: > 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? I was thinking a test case that covers the changes in CodeComplete.cpp e.g. check that Relevance and Quality play well together, and locals/members are boosted? Would that make sense? 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
