[PATCH] D42640: [clangd] Prototype: collect symbol #include & insert #include in global code completion.

Eric Liu via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Tue Feb 6 08:59:05 PST 2018


ioeric added a comment.

Thanks for the comments! I addressed comments in the symbol collect part (I think?). Will add tests in a followup patch.



================
Comment at: clangd/index/HeaderMapCollector.h:48
+  // A map from header patterns to header names.
+  // The header names are not owned. This is only threadsafe because the regexes
+  // never fail.
----------------
sammccall wrote:
> This comment and the mutable are scary. We don't need to be threadsafe I think?
`mutable` was here because of `llvm::Regex`. Matching again `a llvm::Regex` is not a constant operation, but we would still like the lookup function to be `const`.


================
Comment at: clangd/index/Index.h:155
     llvm::StringRef CompletionDetail;
+    /// A URI for the header to be #include'd for this symbol, or a header like
+    /// "<...>" for system headers that are not repository independent (e.g. STL
----------------
sammccall wrote:
> I think we concluded this shouldn't be a URI but a literal include string. If absent, we should fall back to CanonicalDeclaration.
> 
> The reason is that we choose to interpret the target of IWYU directives as a literal string to include. Alternatively we could choose to resolve it somehow and store the URI to the resolved target, but that would be more work.
In our latest offline discussion, we concluded that this would be either URI of a file (in .inc case) or a literal string that is suitable to be inserted. I left resolving IWYU targets as a `FIXME` in this patch as it is more work and doesn't seem to cause any trouble at this point. Will revisit when it actually becomes a problem.


================
Comment at: clangd/index/SymbolCollector.cpp:113
+// to define.
+bool shouldCollectIncludePath(index::SymbolKind Kind) {
+  using SK = index::SymbolKind;
----------------
sammccall wrote:
> shouldn't this just be return SK != Namespace, to avoid duplication? Since we only get here if we're indexing this symbol?
I think it'd be easier to justify the behavior with a white list. There are other symbols like `NamespaceAlias` etc, which we might or might not be collecting. 


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D42640





More information about the cfe-commits mailing list