[PATCH] D64860: [clangd] Get rid of dots and dotsdots within GlobalCompilationDatabase

[Sam McCall via Phabricator via cfe-commits]

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