[PATCH] D53481: [clangd] Support passing a relative path to -compile-commands-dir

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks for the fix!



================
Comment at: clangd/tool/ClangdMain.cpp:261
   } else {
-    CompileCommandsDirPath = CompileCommandsDir;
+    // If the compile-commands-dir arg path is absolute, use it directly. If
+    // the path is relative, try to convert it to an absolute path first.
----------------
This comment echoes the code, but doesn't say *why*.
Consider replacing with
`// clangd changes working directory a lot, so we need an absolute path`.


================
Comment at: clangd/tool/ClangdMain.cpp:263
+    // the path is relative, try to convert it to an absolute path first.
+    if (sys::path::is_absolute(CompileCommandsDir)) {
+      CompileCommandsDirPath = CompileCommandsDir;
----------------
it's fine to call make_absolute on an absolute path, it's a no-op.
You can do that here to avoid a special case.


================
Comment at: clangd/tool/ClangdMain.cpp:267
+      SmallString<128> Path(CompileCommandsDir);
+      std::error_code EC = sys::fs::make_absolute(Path);
+      if (EC) {
----------------
nit: we tend to inline this as `if (std::error_code EC = sys::fs...)`


================
Comment at: clangd/tool/ClangdMain.cpp:272
+               << EC.message() << ". The argument will be ignored.\n";
+        CompileCommandsDirPath = None;
+      } else {
----------------
This is a no-op (as are the existing ones on 255 and 259...)


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D53481