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
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
