sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:77
GlobalCompilationDatabase::getFallbackCommand(PathRef File) const {
+ llvm::SmallString<128> CanonPath(File);
+ llvm::sys::path::remove_dots(CanonPath, true);
----------------
This patch lacks a bit of context (why are we changing this behavior).
Based on offline discussion, it's not clear to me why this particular change to
getFallbackCommand is necessary or desirable. (Compared to others, which seem
like fairly obvious bugs, even if the reason to fix them isn't obvious)
================
Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.cpp:144
"path must be absolute");
+ llvm::SmallString<128> CanonPath(Request.FileName);
+ llvm::sys::path::remove_dots(CanonPath, true);
----------------
nit: can you pull out a local (or FS.h) RemoveDots helper that accepts
StringRef and returns std::string?
Most of the usages in this file could be inlined and I think that would read
better. Not concerned about performance I think. (But returning SmallString is
OK if you are)
================
Comment at:
clang-tools-extra/clangd/unittests/GlobalCompilationDatabaseTests.cpp:307
+ EXPECT_TRUE(DB.getProjectInfo(File));
+ EXPECT_EQ(DB.getFallbackCommand(File).Directory, testRoot());
+}
----------------
Is this actually important/desirable?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D64860/new/
https://reviews.llvm.org/D64860
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
