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

[Kadir Cetinkaya via Phabricator via cfe-commits]

kadircet added inline comments.


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:21
+// file.
+class CollectIndexableLocalDecls {
+public:
----------------
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);` ?


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:61
+
+llvm::Optional<std::string> resolveURI(const char *FileURI,
+                                       llvm::StringRef HintPath) {
----------------
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`?


================
Comment at: clang-tools-extra/clangd/HeaderSourceSwitch.cpp:78
+
+llvm::Optional<Path> getCorrespondingHeaderOrSource(const Path &OriginalFile,
+                                                    ParsedAST &AST,
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67907