[PATCH] D46943: [clangd] Boost scores for decls from current file in completion

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

In https://reviews.llvm.org/D46943#1107880, @ilya-biryukov wrote:

> I've added an initial version of testing for the matching header and wanted to get feedback before proceeding further with tests and other changes.
>
> A few things that bug me so far:
>
> - We need to match headers of items from the index, not only from the Sema results.


This sounds reasonable.

> - Symbols store their paths as URIs ⇒ we need to parse them in order to apply heuristics. We could avoid that by writing a version of header-matching that also works on URIs, but that would mean more complexity.

Yeah, this is a bit unfortunate. It's probably OK to parse URIs; they are not that expensive after all (we might want to do some measurement though).

> - Merging quality signals from headers now requires an extra paramater: name of the source file. I wonder if it's worth extracting builders for symbol qualities into separate classes to keep the current nice signatures, i.e. `merge(Symbol& IndexResult)`.

I'm not very familiar with `SymbolQualitySignals`. But if we decide to use main file name as a signal, it might make sense to pass it in through the constructor?

> - How should we match the header with the main file?  Our options are:
>   - (proposed by Eric) use main file regex from clang-format for that. I'm not entirely sure it suits us well, since the regex is designed to work on paths inside #include directive, but we're getting ours from the Symbols and Sema AST Decls. Moreover, it means we're gonna read .clang-format files to get that style.

I think the ".clang-format problem" is not specific to the header matching here. We would eventually need proper format style support in clangd anyway, as clangd provides formatting features (e.g. reformat and include insertion).

> - Come up with our own heuristics. There is a similar place in ClangdServer that matches a header with source and back. We could extend those heuristics to also allow figuring out whether the paths are matching header/source. I chose this option for initial implementation, since it's less work and it seems easier to switch to clang-format's regex later.

Not against this option. Just want to point out that the heuristics would not work for test files (that include tested main headers) with the current matching.



================
Comment at: clangd/MatchingHeader.cpp:44
+  auto HeaderNoExt = llvm::sys::path::filename(Header);
+  return SourceNoExt.equals_lower(HeaderNoExt);
+}
----------------
Why `equals_lower`?


================
Comment at: clangd/MatchingHeader.h:1
+//===--- MatchingHeader.h - Match source and header files --------*- C++-*-===//
+//
----------------
I wonder if we could merge this into Headers.h


================
Comment at: clangd/MatchingHeader.h:24
+/// header for a \p Source.
+bool isMainHeaderFor(PathRef Header, PathRef Source);
+
----------------
We might want to briefly explain what a matching header is and what the heuristics are.


================
Comment at: clangd/Quality.cpp:28-29
+struct DeclFiles {
+  bool AllDeclsInMainFile = false;
+  bool HasDeclsInMatchingHeader = false;
+};
----------------
Could we merge these two flags into something like `IsDeclaredInMainHeaderOrFile`?


================
Comment at: clangd/Quality.cpp:40
+  assert(MainFile);
+  for (auto *Redecl : D->redecls()) {
+    auto Loc = SM.getSpellingLoc(Redecl->getLocation());
----------------
I wonder if it's still necessary to check all `redecls`. Would it be sufficient to check `D`, if `D` is the decl we referencing to?


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D46943