[PATCH] D44226: [clangd] Add -log-lsp-to-stderr option

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added inline comments.


================
Comment at: clangd/Logger.cpp:19
 Logger *L = nullptr;
+bool Verbose_ = false;
 } // namespace
----------------
simark wrote:
> ilya-biryukov wrote:
> > Could we move the flag to implementation of `Logger`?
> > I.e.:
> > ```
> > class Logger {
> >   virtual log(const llvm::Twine &Message, bool Verbose);
> > };
> > 
> > // Implementation of top-level log
> > void clangd::log(const llvm::Twine &Message) {
> >    L->log(Message, /*Verbose=*/false);
> >    // should also handle missing Logger by logging into llvm::errs()
> > }
> > 
> > // Implementation of top-level vlog.
> > void clangd::vlog(const llvm::Twine &Message) {
> >    L->log(Message, /*Verbose=*/true);
> >    // should also handle missing Logger by logging into llvm::errs()
> > }
> > ```
> > 
> > An implementation of the interface would decide whether to log or not, based on command-line argument.
> That's what I thought of doing first.  The issue is that if we don't set a logger with LoggingSession, the log function prints to stderr itself.  So if we don't check the Verbose flag there, the -v flag will have no effect when LoggingSession has not been called (I don't know if it's really a problem or not, since in practice we call it).
It shouldn't be a problem.
We're only missing `LoggingSession` in tests, so let's just print messages from `vlog` into `stderr` in the same way we do it with `log` now.


================
Comment at: clangd/tool/ClangdMain.cpp:79
 
+static llvm::cl::opt<bool> Verbose("verbose", llvm::cl::desc("Be more verbose"),
+                                   llvm::cl::init(false));
----------------
simark wrote:
> ilya-biryukov wrote:
> > Maybe just call it `-v`?
> I would have like to add both "-v" and "-verbose", but it doesn't seem possible to have two flags for the same option.  "-v" it is then, it is quite standard.
I would go with having just `-v` with no aliases.

But this should do the trick if you prefer to have `-verbose` as an option:
```
llvm::cl::opt<bool> Verbose("v", llvm::cl::alias("verbose") , //....
```


Repository:
  rCTE Clang Tools Extra

https://reviews.llvm.org/D44226