sammccall added a comment.
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?
================
Comment at: clang-tools-extra/clangd/benchmarks/IndexBenchmark.cpp:101
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...",
----------------
this is fine, but just writes to errs with no log formatting... you need to set
up a logger if you want one.
================
Comment at: clang-tools-extra/clangd/index/dex/dexp/Dexp.cpp:185
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;
----------------
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.
================
Comment at: clang-tools-extra/clangd/index/remote/server/Server.cpp:176
std::unique_ptr<grpc::Server> Server(Builder.BuildAndStart());
- llvm::outs() << "Server listening on " << ServerAddress << '\n';
+ vlog("Server listening on {0}", ServerAddress);
----------------
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.
================
Comment at: clang-tools-extra/clangd/indexer/IndexerMain.cpp:116
if (!Executor) {
- llvm::errs() << llvm::toString(Executor.takeError()) << "\n";
+ clang::clangd::elog("{0}", llvm::toString(Executor.takeError()));
return 1;
----------------
unneccesary toString
================
Comment at: clang-tools-extra/clangd/indexer/IndexerMain.cpp:126
if (Err) {
- llvm::errs() << llvm::toString(std::move(Err)) << "\n";
+ clang::clangd::elog("{0}", llvm::toString(std::move(Err)));
}
----------------
unneccesary toString
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:567
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?");
----------------
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)
================
Comment at: clang-tools-extra/clangd/tool/ClangdMain.cpp:624
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
----------------
this isn't an error, and we're explicitly trying to get it in front of the user
*before* the log starts
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D84697/new/
https://reviews.llvm.org/D84697
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
