Page MenuHomePhabricator

[clangd] Upgrade logging facilities with levels and formatv.

Authored by sammccall on Jul 6 2018, 1:40 AM.



log() is split into four functions:

  • elog()/log()/vlog() have different severity levels, allowing filtering
  • dlog() is a lazy macro which uses LLVM_DEBUG - it logs to the logger, but conditionally based on -debug-only flag and is omitted in release builds

All logging functions use formatv-style format strings now, e.g:

log("Could not resolve URI {0}: {1}", URI, Result.takeError());

Existing log sites have been split between elog/log/vlog by best guess.

Diff Detail


Event Timeline

sammccall created this revision.Jul 6 2018, 1:40 AM
sammccall added subscribers: simark, malaperle.

This borrows from D44226 (log levels) which got stalled for various reasons (@simark - is it OK if I pick this up?).
It doesn't actually split the LSP messages vs LSP message names into different levels, but that's a simple followup.

Switching to formatv seems like a nice ergonomic win while we're breaking interfaces.
It also in principle allows loggers to extract the format string and args separately (formatv doesn't yet provide APIs for this, but architecturally it's easy) which can help log analysis.

hokein accepted this revision.Jul 6 2018, 5:19 AM

Nice, look good.

104 ↗(On Diff #154372)

Is it intended not to expose Verbose mode?

This revision is now accepted and ready to land.Jul 6 2018, 5:19 AM
sammccall added inline comments.Jul 6 2018, 5:28 AM
104 ↗(On Diff #154372)

dlog is already controlled by -debug/-debug-only flag, so it seems it doesn't need a distinct level here too. WDYT?

Renamed flag value to "verbose".

sammccall updated this revision to Diff 154391.Jul 6 2018, 5:28 AM

Name debug->verbose

hokein added a comment.Jul 6 2018, 5:57 AM

still LGTM.

The diff contains unrelated changes now.

104 ↗(On Diff #154372)

Yeah, vlog is controlled by debug flag. A flag "verbose" is more natural than debug.

simark added a comment.Jul 6 2018, 8:16 AM

@simark - is it OK if I pick this up?

Yes, totally fine. I didn't have time to follow up on my patch, and I think you did a great job here. I tested it, and I see that my main pain point was addressed. With the patch, clangd is more quiet by default, only outputting errors, so I'm happy :).

sammccall updated this revision to Diff 154959.Jul 11 2018, 3:38 AM

Add workaround for formatv+error issues.

This revision was automatically updated to reflect the committed changes.