ioeric added inline comments.
================ 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 ---------------- sammccall wrote: > 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. So it's just 2 lines for the replacement magic (+1 for comment) now after removing some redundant code. I like `shortenIncludePath` better because it's more self-contained and easier to write tests against, and `insertInclude` doesn't seem to carry much more weight while we would need to handle `replacements` logic which has been tested in the tests. ================ Comment at: clangd/ClangdServer.cpp:370 +/// File, by matching \p Header against all include search directories for \p +/// File via `clang::HeaderSearch`. +/// ---------------- sammccall wrote: > 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 I added a comment for this in the header. But I might be misunderstanding you suggestion. Did you mean we need a better name for the function? 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
