ilya-biryukov added inline comments.
================ Comment at: clangd/GlobalCompilationDatabase.cpp:75 + if (CachedIt != CompilationDatabases.end()) + return (CachedIt->second.get()); + auto CDB = tooling::CompilationDatabase::loadFromDirectory(File, Error); ---------------- Parentheses seem redundant. ================ Comment at: clangd/GlobalCompilationDatabase.cpp:78 + if (CDB && Error.empty()) { + auto result = CDB.get(); + CompilationDatabases.insert(std::make_pair(File, std::move(CDB))); ---------------- Code style: local variable are `UpperCamelCase` ================ Comment at: clangd/GlobalCompilationDatabase.cpp:96 + if (!CompileCommandsDir.empty()) { + auto CDB = tryLoadDatabaseFromPath(CompileCommandsDir); + return CDB; ---------------- Even better: `return tryLoadDatabaseFromPath(CompileCommandsDir); ` ================ Comment at: clangd/GlobalCompilationDatabase.h:50 + llvm::Optional<Path> NewCompileCommandsDir) + : CompileCommandsDir(NewCompileCommandsDir.getValue()) {} std::vector<tooling::CompileCommand> ---------------- `getValue()` will fail if NewCompileCommandsDir does not have a value. The original suggestion was to change type of the field 'CompileCommandsDir` to `llvm::Optional<Path>` and check if `Optional` has a value instead of checking for empty string. ================ Comment at: clangd/GlobalCompilationDatabase.h:58 tooling::CompilationDatabase *getCompilationDatabase(PathRef File); + Path CompileCommandsDir; + tooling::CompilationDatabase *tryLoadDatabaseFromPath(PathRef File); ---------------- Could you please move this to other fields? Mixing functions and fields is a bit confusing. ================ Comment at: clangd/tool/ClangdMain.cpp:52 + if (!llvm::sys::fs::exists(CompileCommandsDir)) { + Out.log("Path specified by --compile-commands-dir. The argument will be " + "ignored.\n"); ---------------- Message does not mention that path does not exist. Probably incidental. ================ Comment at: clangd/tool/ClangdMain.cpp:59 // Change stdin to binary to not lose \r\n on windows. llvm::sys::ChangeStdinToBinary(); + if (CompileCommandsDir.empty()) ---------------- Could you please move this out of the logic that validates `CompileCommandsDir` parameter? It's very easy to miss while reading the code. Probably ok to put it right after `JSONOutput Out(...` line https://reviews.llvm.org/D37150 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits