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