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