gribozavr accepted this revision.
gribozavr added a comment.
This revision is now accepted and ready to land.
I'm not an expert in this code, but it looks reasonable.
================
Comment at: clang-tools-extra/clangd/Headers.h:155
+ /// \param IncludingFile Used to shorten the include for headers in the same
+ /// directory.
+ ///
----------------
Please don't write what something is used for, write what it is. You can
explain how it affects the output of the function of course, but don't just say
"used for" -- like what you did in clang/include/clang/Lex/HeaderSearch.h.
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:215
TEST_F(HeadersTest, ShortenedInclude) {
std::string BarHeader = testPath("sub/bar.h");
----------------
This test name should probably mention the fact that the directory is on the
include path.
================
Comment at: clang-tools-extra/clangd/unittests/HeadersTests.cpp:225
TEST_F(HeadersTest, NotShortenedInclude) {
std::string BarHeader =
----------------
The test name probably should be adjusted.
================
Comment at: clang/lib/Lex/HeaderSearch.cpp:1683
unsigned BestPrefixLength = 0;
- unsigned BestSearchDir;
-
- for (unsigned I = 0; I != SearchDirs.size(); ++I) {
- // FIXME: Support this search within frameworks and header maps.
- if (!SearchDirs[I].isNormalDir())
- continue;
-
- StringRef Dir = SearchDirs[I].getDir()->getName();
+ auto CheckDir = [&](llvm::StringRef Dir) {
llvm::SmallString<32> DirPath(Dir.begin(), Dir.end());
----------------
I'd suggest to spell the return type, and document what it means.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D63295/new/
https://reviews.llvm.org/D63295
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
