[clangd] Use elog instead of llvm::errs, log instead of llvm::outs

[kbobyrev]

Don't use llvm::toString in elog

[sammccall]

Thanks for sending this... after looking at the examples and thinking again about our conversation I may have shifted a bit :-\

I think it's important *log() should only be used for messages intended to be logged! (i.e. read later as a sequence of log messages).
We happen to write these to stderr by default, but this is an implementation detail that I'd like to leave flexible.

So a good question is: "if we were writing the log to a different file instead, would we want to write this there or stderr?"
By that measure:

• the messages followed by exit() should go to stderr
• dexp's output should go to stder
• the others seem good to go to the log

WDYT?

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

101

this is fine, but just writes to errs with no log formatting... you need to set up a logger if you want one.

clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp

185

hmm... since this is explicitly supposed to be an interactive tool, I think "we are writing to stderr" is important rather than a detail to be abstracted.

e.g. there's no timestamp etc decoration (which is good!) because you haven't installed an actual logger and the fallback behavior is to write to stderr. But neither of these things are explicit at the callsite and either could be changed elsewhere.

clang-tools-extra/clangd/index/remote/server/Server.cpp

176

I'd suggest this is log() rather than vlog() - the port is sometimes important to know but *progress through the startup sequence* is useful to have in the log.

clang-tools-extra/clangd/indexer/IndexerMain.cpp

116

unneccesary toString

126

unneccesary toString

clang-tools-extra/clangd/tool/ClangdMain.cpp

567

Don't use elog before the logger is set up to print messages to stderr - it seems needlessly confusing where the current version is very clear.

(it *is* better for the basic validation to happen early and us to log flag failures "before the stream enters log mode" where it's convenient)

624

this isn't an error, and we're explicitly trying to get it in front of the user *before* the log starts

[kbobyrev]

Use more conservative approach.

[sammccall]

clang-tools-extra/clangd/index/remote/server/Server.cpp

193

explain why or just delete this comment

193

nit: move this down after flag validation?

195

I'm not sure performance matters here (if you don't actually write to stdout much, it should be free). Rather, this is for correctness: it's not threadsafe.

[kbobyrev]

Resolve post-LGTM comments.

clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp

 //===--- IndexBenchmark.cpp - Clangd index benchmarks -----------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../index/Serialization.h"
 #include "../index/dex/Dex.h"
 #include "benchmark/benchmark.h"
+#include "support/Logger.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/Regex.h"
 #include <fstream>
 #include <streambuf>
 #include <string>
 
std::vector<FuzzyFindRequest> extractQueriesFromLogs() {
   std::string Log((std::istreambuf_iterator<char>(InputStream)),
                   std::istreambuf_iterator<char>());
 
   std::vector<FuzzyFindRequest> Requests;
   auto JSONArray = llvm::json::parse(Log);
 
   // Panic if the provided file couldn't be parsed.
   if (!JSONArray) {
-    llvm::errs() << "Error when parsing JSON requests file: "
-                 << llvm::toString(JSONArray.takeError());
+    elog("Error when parsing JSON requests file: {0}", JSONArray.takeError());
     exit(1);
   }
   if (!JSONArray->getAsArray()) {
-    llvm::errs() << "Error: top-level value is not a JSON array: " << Log
-                 << '\n';
+    elog("Error: top-level value is not a JSON array: {0}", Log);
     exit(1);
   }
 
   for (const auto &Item : *JSONArray->getAsArray()) {
     FuzzyFindRequest Request;
     // Panic if the provided file couldn't be parsed.
     if (!fromJSON(Item, Request)) {
-      llvm::errs() << "Error when deserializing request: " << Item << '\n';
+      elog("Error when deserializing request: {0}", Item);
       exit(1);
     }
     Requests.push_back(Request);
   }
   return Requests;
 }
 
 static void MemQueries(benchmark::State &State) {

 } // namespace clang
 
 // FIXME(kbobyrev): Add index building time benchmarks.
 // FIXME(kbobyrev): Add memory consumption "benchmarks" by manually measuring
 // in-memory index size and reporting it as time.
 // FIXME(kbobyrev): Create a logger wrapper to suppress debugging info printer.
 int main(int argc, char *argv[]) {
   if (argc < 3) {
-    llvm::errs() << "Usage: " << argv[0]
-                 << " global-symbol-index.yaml requests.json "
-                    "BENCHMARK_OPTIONS...\n";
+    clang::clangd::elog("Usage: {0} global-symbol-index.yaml requests.json "
+                        "BENCHMARK_OPTIONS...",
+                        argv[0]);
     return -1;
   }
   IndexFilename = argv[1];
   RequestsFilename = argv[2];
   // Trim first two arguments of the benchmark invocation and pretend no
   // arguments were passed in the first place.
   argv[2] = argv[0];
   argv += 2;
   argc -= 2;
   ::benchmark::Initialize(&argc, argv);
   ::benchmark::RunSpecifiedBenchmarks();
 }

clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp

 //===--- Dexp.cpp - Dex EXPloration tool ------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements a simple interactive tool which can be used to manually
 // evaluate symbol search quality of Clangd index.
 //
 //===----------------------------------------------------------------------===//
 
 #include "index/Serialization.h"
 #include "index/dex/Dex.h"
 #include "index/remote/Client.h"
+#include "support/Logger.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/LineEditor/LineEditor.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Signals.h"
 
 namespace clang {
class Lookup : public Command {
   };
   llvm::cl::opt<std::string> Name{
       "name",
       llvm::cl::desc("Qualified name to look up."),
   };
 
   void run() override {
     if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) {
-      llvm::errs()
-          << "Missing required argument: please provide id or -name.\n";
+      elog("Missing required argument: please provide id or -name.");
       return;
     }
     std::vector<SymbolID> IDs;
     if (ID.getNumOccurrences()) {
       auto SID = SymbolID::fromStr(ID);
       if (!SID) {
-        llvm::errs() << llvm::toString(SID.takeError()) << "\n";
+        elog("{0}", SID.takeError());
         return;
       }
       IDs.push_back(*SID);
     } else {
       IDs = getSymbolIDsFromIndex(Name, Index);
     }
 
     LookupRequest Request;
     Request.IDs.insert(IDs.begin(), IDs.end());
     bool FoundSymbol = false;
     Index->lookup(Request, [&](const Symbol &Sym) {
       FoundSymbol = true;
       llvm::outs() << toYAML(Sym);
     });
     if (!FoundSymbol)
-      llvm::errs() << "not found\n";
+      elog("not found");
   }
 };
 
 class Refs : public Command {
   llvm::cl::opt<std::string> ID{
       "id",
       llvm::cl::Positional,
       llvm::cl::desc("Symbol ID of the symbol being queried (hex)."),
   };
   llvm::cl::opt<std::string> Name{
       "name",
       llvm::cl::desc("Qualified name of the symbol being queried."),
   };
   llvm::cl::opt<std::string> Filter{
       "filter",
       llvm::cl::init(".*"),
       llvm::cl::desc(
           "Print all results from files matching this regular expression."),
   };
 
   void run() override {
     if (ID.getNumOccurrences() == 0 && Name.getNumOccurrences() == 0) {
-      llvm::errs()
-          << "Missing required argument: please provide id or -name.\n";
+      elog("Missing required argument: please provide id or -name.");
       return;
     }
     std::vector<SymbolID> IDs;
     if (ID.getNumOccurrences()) {
       auto SID = SymbolID::fromStr(ID);
       if (!SID) {
-        llvm::errs() << llvm::toString(SID.takeError()) << "\n";
+        elog("{0}", SID.takeError());
         return;
       }
       IDs.push_back(*SID);
     } else {
       IDs = getSymbolIDsFromIndex(Name, Index);
       if (IDs.size() > 1) {
-        llvm::errs() << llvm::formatv(
-            "The name {0} is ambiguous, found {1} different "
-            "symbols. Please use id flag to disambiguate.\n",
+        elog("The name {0} is ambiguous, found {1} different symbols. Please "
+             "use id flag to disambiguate.",
              Name, IDs.size());
         return;
       }
     }
     RefsRequest RefRequest;
     RefRequest.IDs.insert(IDs.begin(), IDs.end());
     llvm::Regex RegexFilter(Filter);
     Index->refs(RefRequest, [&RegexFilter](const Ref &R) {
       auto U = URI::parse(R.Location.FileURI);
       if (!U) {
-        llvm::errs() << U.takeError();
+        elog("{0}", U.takeError());
         return;
       }
       if (RegexFilter.match(U->body()))
         llvm::outs() << R << "\n";
     });
   }
 };
 
class Export : public Command {
   };
 
 public:
   void run() {
     using namespace clang::clangd;
     // Read input file (as specified in global option)
     auto Buffer = llvm::MemoryBuffer::getFile(IndexLocation);
     if (!Buffer) {
-      llvm::errs() << llvm::formatv("Can't open {0}", IndexLocation) << "\n";
+      elog("Can't open {0}", IndexLocation);
       return;
     }
 
     // Auto-detects input format when parsing
     auto IndexIn = clang::clangd::readIndexFile(Buffer->get()->getBuffer());
     if (!IndexIn) {
-      llvm::errs() << llvm::toString(IndexIn.takeError()) << "\n";
+      elog("{0}", IndexIn.takeError());
       return;
     }
 
     // Prepare output file
     std::error_code EC;
     llvm::raw_fd_ostream OutputStream(OutputFile, EC);
     if (EC) {
-      llvm::errs() << llvm::formatv("Can't open {0} for writing", OutputFile)
-                   << "\n";
+      elog("Can't open {0} for writing", OutputFile);
       return;
     }
 
     // Export
     clang::clangd::IndexFileOut IndexOut(IndexIn.get());
     IndexOut.Format = Format;
     OutputStream << IndexOut;
   }
bool runCommand(std::string Request, const SymbolIndex &Index) {
   for (llvm::StringRef S : Args)
     FakeArgv.push_back(S.data()); // Terminated by separator or end of string.
 
   for (const auto &Cmd : CommandInfo) {
     if (Cmd.Name == Args.front())
       return Cmd.Implementation()->parseAndRun(FakeArgv, Cmd.Description,
                                                Index);
   }
-  llvm::errs() << "Unknown command. Try 'help'.\n";
+  elog("Unknown command. Try 'help'");
   return false;
 }
 
 } // namespace
 } // namespace clangd
 } // namespace clang
 
 int main(int argc, const char *argv[]) {
   using namespace clang::clangd;
 
   llvm::cl::ParseCommandLineOptions(argc, argv, Overview);
   llvm::cl::ResetCommandLineParser(); // We reuse it for REPL commands.
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   std::unique_ptr<SymbolIndex> Index;
   reportTime(llvm::StringRef(IndexLocation).startswith("remote:")
                  ? "Remote index client creation"
                  : "Dex build",
              [&]() { Index = openIndex(IndexLocation); });
 
   if (!Index) {
-    llvm::errs() << "Failed to open the index.\n";
+    elog("Failed to open the index.");
     return -1;
   }
 
   if (!ExecCommand.empty())
     return runCommand(ExecCommand, *Index) ? 0 : 1;
 
   llvm::LineEditor LE("dexp");
   while (llvm::Optional<std::string> Request = LE.readLine())
     runCommand(std::move(*Request), *Index);
 }

clang-tools-extra/clangd/index/remote/server/Server.cpp

 //===--- Server.cpp - gRPC-based Remote Index Server  ---------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "Index.pb.h"
 #include "index/Index.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/remote/marshalling/Marshalling.h"
 #include "support/Logger.h"
 #include "support/Trace.h"
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/CommandLine.h"
void runServer(std::unique_ptr<clangd::SymbolIndex> Index,
                const std::string &ServerAddress) {
   RemoteIndexServer Service(std::move(Index), IndexRoot);
 
   grpc::EnableDefaultHealthCheckService(true);
   grpc::ServerBuilder Builder;
   Builder.AddListeningPort(ServerAddress, grpc::InsecureServerCredentials());
   Builder.RegisterService(&Service);
   std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
-  llvm::outs() << "Server listening on " << ServerAddress << '\n';
+  vlog("Server listening on {0}", ServerAddress);
 
   Server->Wait();
 }
 
 } // namespace
 } // namespace remote
 } // namespace clangd
 } // namespace clang
 
 using clang::clangd::elog;
 
 int main(int argc, char *argv[]) {
   using namespace clang::clangd::remote;
   llvm::cl::ParseCommandLineOptions(argc, argv, Overview);
   llvm::sys::PrintStackTraceOnErrorSignal(argv[0]);
 
   if (!llvm::sys::path::is_absolute(IndexRoot)) {
     elog("Index root should be an absolute path.");
     return -1;
   }
 
   llvm::Optional<llvm::raw_fd_ostream> TracerStream;
   std::unique_ptr<clang::clangd::trace::EventTracer> Tracer;
   if (!TraceFile.empty()) {
     std::error_code EC;
     TracerStream.emplace(TraceFile, EC,
                          llvm::sys::fs::FA_Read | llvm::sys::fs::FA_Write);

clang-tools-extra/clangd/indexer/IndexerMain.cpp

 //===----------------------------------------------------------------------===//
 
 #include "index/IndexAction.h"
 #include "index/Merge.h"
 #include "index/Ref.h"
 #include "index/Serialization.h"
 #include "index/Symbol.h"
 #include "index/SymbolCollector.h"
+#include "support/Logger.h"
 #include "clang/Tooling/ArgumentsAdjusters.h"
 #include "clang/Tooling/CommonOptionsParser.h"
 #include "clang/Tooling/Execution.h"
 #include "clang/Tooling/Tooling.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Signals.h"
 
 namespace clang {
int main(int argc, const char **argv) {
 
   Note: only symbols from header files will be indexed.
   )";
 
   auto Executor = clang::tooling::createExecutorFromCommandLineArgs(
       argc, argv, llvm::cl::GeneralCategory, Overview);
 
   if (!Executor) {
-    llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
+    clang::clangd::elog("{0}", llvm::toString(Executor.takeError()));
     return 1;
   }
 
   // Collect symbols found in each translation unit, merging as we go.
   clang::clangd::IndexFileIn Data;
   auto Err = Executor->get()->execute(
       std::make_unique<clang::clangd::IndexActionFactory>(Data),
       clang::tooling::getStripPluginsAdjuster());
   if (Err) {
-    llvm::errs() << llvm::toString(std::move(Err)) << "\n";
+    clang::clangd::elog("{0}", llvm::toString(std::move(Err)));
   }
 
   // Emit collected data.
   clang::clangd::IndexFileOut Out(Data);
   Out.Format = clang::clangd::Format;
   llvm::outs() << Out;
   return 0;
 }

clang-tools-extra/clangd/tool/ClangdMain.cpp

else if (EnableBackgroundIndex)
       BackgroundQueue::preventThreadStarvationInTests();
   }
   if (Test || EnableTestScheme) {
     static URISchemeRegistry::Add<TestScheme> X(
         "test", "Test scheme for clangd lit tests.");
   }
 
   if (!Sync && WorkerThreadsCount == 0) {
-    llvm::errs() << "A number of worker threads cannot be 0. Did you mean to "
-                    "specify -sync?";
+    elog("A number of worker threads cannot be 0. Did you mean to "
+         "specify -sync?");
     return 1;
   }
 
   if (Sync) {
     if (WorkerThreadsCount.getNumOccurrences())
-      llvm::errs() << "Ignoring -j because -sync is set.\n";
+      elog("Ignoring -j because -sync is set.");
     WorkerThreadsCount = 0;
   }
   if (FallbackStyle.getNumOccurrences())
     clang::format::DefaultFallbackStyle = FallbackStyle.c_str();
 
   // Validate command line arguments.
   llvm::Optional<llvm::raw_fd_ostream> InputMirrorStream;
   if (!InputMirrorFile.empty()) {
     std::error_code EC;
     InputMirrorStream.emplace(InputMirrorFile, /*ref*/ EC,
                               llvm::sys::fs::FA_Read | llvm::sys::fs::FA_Write);
     if (EC) {
       InputMirrorStream.reset();
-      llvm::errs() << "Error while opening an input mirror file: "
-                   << EC.message();
+      elog("Error while opening an input mirror file: {0}", EC.message());
     } else {
       InputMirrorStream->SetUnbuffered();
     }
   }
 
   // Setup tracing facilities if CLANGD_TRACE is set. In practice enabling a
   // trace flag in your editor's config is annoying, launching with
   // `CLANGD_TRACE=trace.json vim` is easier.
   llvm::Optional<llvm::raw_fd_ostream> TracerStream;
   std::unique_ptr<trace::EventTracer> Tracer;
   const char *JSONTraceFile = getenv("CLANGD_TRACE");
   const char *MetricsCSVFile = getenv("CLANGD_METRICS");
   const char *TracerFile = JSONTraceFile ? JSONTraceFile : MetricsCSVFile;
   if (TracerFile) {
     std::error_code EC;
     TracerStream.emplace(TracerFile, /*ref*/ EC,
                          llvm::sys::fs::FA_Read | llvm::sys::fs::FA_Write);
     if (EC) {
       TracerStream.reset();
-      llvm::errs() << "Error while opening trace file " << TracerFile << ": "
-                   << EC.message();
+      elog("Error while opening trace file {0}: {1}", TracerFile, EC.message());
     } else {
       Tracer = (TracerFile == JSONTraceFile)
                    ? trace::createJSONTracer(*TracerStream, PrettyPrint)
                    : trace::createCSVMetricTracer(*TracerStream);
     }
   }
 
   llvm::Optional<trace::Session> TracingSession;
   if (Tracer)
     TracingSession.emplace(*Tracer);
 
   // If a user ran `clangd` in a terminal without redirecting anything,
   // it's somewhat likely they're confused about how to use clangd.
   // Show them the help overview, which explains.
   if (llvm::outs().is_displayed() && llvm::errs().is_displayed())
-    llvm::errs() << Overview << "\n";
+    elog(Overview);
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
   llvm::errs().SetBuffered();
   // Don't flush stdout when logging, this would be both slow and racy!
   llvm::errs().tie(nullptr);
   StreamLogger Logger(llvm::errs(), LogLevel);
   LoggingSession LoggingSession(Logger);
   // Write some initial logs before we start doing any real work.
)";
   if (!CompileCommandsDir.empty()) {
     if (llvm::sys::fs::exists(CompileCommandsDir)) {
       // We support passing both relative and absolute paths to the
       // --compile-commands-dir argument, but we assume the path is absolute in
       // the rest of clangd so we make sure the path is absolute before
       // continuing.
       llvm::SmallString<128> Path(CompileCommandsDir);
       if (std::error_code EC = llvm::sys::fs::make_absolute(Path)) {
-        llvm::errs() << "Error while converting the relative path specified by "
-                        "--compile-commands-dir to an absolute path: "
-                     << EC.message() << ". The argument will be ignored.\n";
+        elog("Error while converting the relative path specified by "
+             "--compile-commands-dir to an absolute path: {0}. The argument "
+             "will be ignored.",
+             EC.message());
       } else {
         CompileCommandsDirPath = std::string(Path.str());
       }
     } else {
-      llvm::errs()
-          << "Path specified by --compile-commands-dir does not exist. The "
-             "argument will be ignored.\n";
+      elog("Path specified by --compile-commands-dir does not exist. The "
+           "argument will be ignored.");
     }
   }
 
   ClangdServer::Options Opts;
   switch (PCHStorage) {
   case PCHStorageFlag::Memory:
     Opts.StorePreamblesInMemory = true;
     break;
AsyncIndexLoad = runAsync<void>([Placeholder] {
       if (auto Idx = loadIndex(IndexFile, /*UseDex=*/true))
         Placeholder->reset(std::move(Idx));
     });
     if (Sync)
       AsyncIndexLoad.wait();
   }
 #if CLANGD_ENABLE_REMOTE
   if (RemoteIndexAddress.empty() != ProjectRoot.empty()) {
-    llvm::errs() << "remote-index-address and project-path have to be "
-                    "specified at the same time.";
+    elog("remote-index-address and project-path have to be specified at the "
+         "same time.");
     return 1;
   }
   if (!RemoteIndexAddress.empty()) {
     if (IndexFile.empty()) {
       log("Connecting to remote index at {0}", RemoteIndexAddress);
       StaticIdx = remote::getClient(RemoteIndexAddress, ProjectRoot);
       EnableBackgroundIndex = false;
     } else {
if (EnableConfig) {
     if (llvm::sys::path::user_config_directory(UserConfig)) {
       llvm::sys::path::append(UserConfig, "clangd", "config.yaml");
       vlog("User config file is {0}", UserConfig);
       ProviderStack.push_back(config::Provider::fromYAMLFile(UserConfig, TFS));
     } else {
       elog("Couldn't determine user config file, not loading");
     }
     std::vector<const config::Provider *> ProviderPointers;
-    for (const auto& P : ProviderStack)
+    for (const auto &P : ProviderStack)
       ProviderPointers.push_back(P.get());
     Config = config::Provider::combine(std::move(ProviderPointers));
     Opts.ConfigProvider = Config.get();
   }
 
   // Initialize and run ClangdLSPServer.
   // Change stdin to binary to not lose \r\n on windows.
   llvm::sys::ChangeStdinToBinary();
 
   std::unique_ptr<Transport> TransportLayer;
   if (getenv("CLANGD_AS_XPC_SERVICE")) {
 #if CLANGD_BUILD_XPC
     log("Starting LSP over XPC service");
     TransportLayer = newXPCTransport();
 #else
-    llvm::errs() << "This clangd binary wasn't built with XPC support.\n";
+    elog("This clangd binary wasn't built with XPC support.");
     return (int)ErrorResultCode::CantRunAsXPCService;
 #endif
   } else {
     log("Starting LSP over stdin/stdout");
     TransportLayer = newJSONTransport(
         stdin, llvm::outs(),
         InputMirrorStream ? InputMirrorStream.getPointer() : nullptr,
         PrettyPrint, InputStyle);