[PATCH] D67907: [clangd] Implement a smart version of HeaderSource switch.

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:21
+// file.
+class CollectIndexableLocalDecls {
+public:
----------------
kadircet wrote:
> this class doesn't seem to have any state(apart from saving AST in constructor and using it in call to collect), why not just have a `vector<Decl> collectDecls(ParsedAST& AST);` ?
yeah, but we need to do recursive calls, which I thought making a class is more readable. Change to a single function with recursive lambda, I hope it won't hurt the code readability.


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:61
+
+llvm::Optional<std::string> resolveURI(const char *FileURI,
+                                       llvm::StringRef HintPath) {
----------------
kadircet wrote:
> It seems like we have a bunch of different implementations for this function in background.cpp, codecomplete.cpp, ... Basically any place calling `URI::resolve`, do you mind adding an overload for `URI::resolve` that'll take a `const char*` instead of a `URI`?
Fair suggestion, done in r372617.


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:78
+
+llvm::Optional<Path> getCorrespondingHeaderOrSource(const Path &OriginalFile,
+                                                    ParsedAST &AST,
----------------
kadircet wrote:
> As discussed in the design for define out-of-line, this function's dependency on AST and Index should rather be optional.
> 
> - if we have AST, then we can boost the files containing the canonical declarations for symbols defined in the main file.
> - if we have just the Index, currently there's nothing much we can do, but we can put a fixme to add a new endpoint to symbol index for "fetching symbols in a given file",
>   then we can use symbol information for declaration/definition files without an AST.
> - if we have AST+Index, we can boost the files containing the definition for symbols declared in the main file and vice-versa(what you already do in this patch).
> - If we don't have anything we'll just make use of the filename, by changing the extension, as current `ClangdServer::switchSourceHeader` implementation does.
as discussed offline, we will
- make the filename heuristic into a separate API which provides more flexible;
- make this API support 3 cases (except the case where we only have index, as it'd require large effort to implement "fetching symbols in a given file" in the index, we're less certain it is worth); when we only have AST (or the index doesn't contain any interesting information), we just use the information for the AST to do the ".cc=>.h" inference.

  



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67907/new/

https://reviews.llvm.org/D67907