sammccall added a comment.
Insertion side LGTM, feel free to split and land.
Sorry I need to take off and will need to get to indexing on monday :(
================
Comment at: clangd/ClangdServer.cpp:368
+/// Calculates the shortest possible include path when inserting \p Header to
\p
+/// File, by matching \p Header against all include search directories for \p
----------------
ioeric wrote:
> sammccall wrote:
> > This has a fair amount of logic (well, plumbing :-D) and uses relatively
> > little from ClangdServer.
> > Can we move `shortenIncludePath` to a separate file and pass in the FS and
> > command?
> >
> > I'd suggest maybe `Headers.h` - The formatting part of `insertInclude`
> > could also fit there, as well as probably the logic from
> > `switchSourceHeader` I think (the latter not in this patch of course).
> >
> > This won't really help much with testing, I just find it gets hard to
> > navigate files that have lots of details of unrelated features. ClangdUnit
> > and ClangdServer both have this potential to grow without bound, though
> > they're in reasonable shape now. Interested what you think!
> Done.
>
> I didn't move the formatting code, as half of the code is pulling out the
> style, which we might want to share/change depending on other clangd logics
> that might use style.
I'd still think pulling out `Expected<tooling::Replacement> insertInclude(File,
Code, Header, VFS, Style)` would be worthwhile here - the formatting isn't a
lot of code, but it's a bit magic, plus the quote handling... it's a bit of
code. It'd make it more obvious what the interactions with ClangdServer's state
are. But up to you, we can always do this later.
================
Comment at: clangd/ClangdServer.cpp:370
+/// File, by matching \p Header against all include search directories for \p
+/// File via `clang::HeaderSearch`.
+///
----------------
sammccall wrote:
> maybe drop "via clang::HeaderSearch" (it's doc'd in the implementation) and
> add an example?
>
> It might be worth explicitly stating that the result is an #include string
> (with quotes) - you just call it a "path" here.
>
> "shortest" makes sense as a first implementation, but we may want to document
> something more like "best" - I know there are codebases we care about where
> file-relative paths are banned. Also I suspect we don't give
> "../../../../Foo.h" even if it's shortest :-)
> "shortest" makes sense as a first implementation, but we may want to document
> something more like "best" - I know there are codebases we care about where
> file-relative paths are banned. Also I suspect we don't give
> "../../../../Foo.h" even if it's shortest :-)
I think this part wasn't addressed
================
Comment at: clangd/Headers.cpp:74
+
+ // We don't need to provide the content of \p File, as we are only interested
+ // in the include search directories in the compile command.
----------------
comment is outdated now
================
Comment at: clangd/Headers.cpp:114
+
+ log("Suggested #include is: " + Suggested);
+ return Suggested;
----------------
can you include the Header in this log message? (and possibly File, but that
might add more noise than signal)
================
Comment at: clangd/Headers.h:30
+/// \return A quoted "path" or <path>. If \p Header is already (directly)
+/// included in the file (including those included via different paths), an
+/// error will be returned.
----------------
header-already-included is not an error condition.
Suggest returning llvm::Expected<Optional<String>>, or returning "" for this
case.
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
