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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits