Page MenuHomePhabricator

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

Authored by kbobyrev on Jul 27 2020, 12:53 PM.

Diff Detail

Event Timeline

kbobyrev created this revision.Jul 27 2020, 12:53 PM
kbobyrev updated this revision to Diff 281027.Jul 27 2020, 12:54 PM

Don't use llvm::toString in elog

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 ↗(On Diff #281027)

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 ↗(On Diff #281027)

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
185

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
568

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)

626

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

kbobyrev updated this revision to Diff 281130.Jul 28 2020, 12:32 AM
kbobyrev marked 7 inline comments as done.

Use more conservative approach.

sammccall accepted this revision.Jul 28 2020, 7:56 AM
sammccall added inline comments.
clang-tools-extra/clangd/index/remote/server/Server.cpp
202

explain why or just delete this comment

202

nit: move this down after flag validation?

204

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.

This revision is now accepted and ready to land.Jul 28 2020, 7:56 AM
kbobyrev updated this revision to Diff 281416.Jul 28 2020, 4:40 PM
kbobyrev marked 3 inline comments as done.

Resolve post-LGTM comments.

This revision was landed with ongoing or failed builds.Jul 28 2020, 4:48 PM
This revision was automatically updated to reflect the committed changes.