ilya-biryukov added inline comments.
================
Comment at: clangd/ClangdLSPServer.cpp:412
+ llvm::raw_string_ostream OS(Message);
+ OS << "method not found (" << Method << ")";
+ replyError(ErrorCode::MethodNotFound, OS.str());
----------------
simark wrote:
> simark wrote:
> > ilya-biryukov wrote:
> > > malaperle wrote:
> > > > simark wrote:
> > > > > ilya-biryukov wrote:
> > > > > > Could we also `log` (i.e. not `vlog`) names of the methods that
> > > > > > were handled successfully? To have some context when something
> > > > > > crashes and we only have non-verbose logs.
> > > > > That would be against the purpose of this patch, which is to output
> > > > > nothing if everything goes right (so it's easier to notice when
> > > > > something goes wrong). Just outputting the names of the methods that
> > > > > are handled successfully would still be very verbose I think.
> > > > Wouldn't that mean pretty much logging everything coming in? The idea
> > > > of this patch it to make it that by default Clangd is not talkative
> > > > unless there is an error and optionally make it verbose, for
> > > > troubleshooting.
> > > Logs are also useful to diagnose problems in case something crashes or
> > > works incorrectly.
> > > Clang will probably log something when processing any request anyway,
> > > logging the method name first will merely give some more context on which
> > > request other log messages correspond to.
> > >
> > > > The idea of this patch it to make it that by default Clangd is not
> > > > talkative unless there is an error.
> > > I don't think we use logging this way in clangd. Logs give us a way to
> > > make sense of what's happening inside clangd even when there's no error.
> > > `vlog` lets us turn off some extremely noisy output that is not useful
> > > most of the time.
> > > We have a whole bunch of log statements that don't correspond to errors
> > > (e.g. "reusing preamble", "building file with compile command").
> > > Logs are also useful to diagnose problems in case something crashes or
> > > works incorrectly.
> > Clang will probably log something when processing any request anyway,
> > logging the method name first will merely give some more context on which
> > request other log messages correspond to.
> >
> > I think it's fine if clangd logs warning/errors by default, that a user
> > might want to look at and address. But logging things that happen
> > recurrently and are part of clangd's normal operation just flood the
> > terminal (and makes it harder to spot actual errors).
> >
> > I agree that having some context helps to make sense of an error. A
> > reasonnable way would be to include the method name only when printing an
> > error. For example, `While handling method 'workspace/symbol': Could not
> > open file foo.`. That would require us to carry around some more context
> > information, but I think it would be better than printing all the handled
> > methods.
> >
> > > I don't think we use logging this way in clangd. Logs give us a way to
> > > make sense of what's happening inside clangd even when there's no error.
> >
> > In that case you turn on a more verbose log level.
> >
> > > vlog lets us turn off some extremely noisy output that is not useful most
> > > of the time. We have a whole bunch of log statements that don't
> > > correspond to errors (e.g. "reusing preamble", "building file with
> > > compile command").
> >
> > I would consider everything that happens at each json-rpc request to be
> > "noisy". When using clangd from an IDE, there's quite a lot of requests
> > being done. I was also thinking of modyfing the patch to also use vlog for
> > the "reusing preamble" and "building file with compile command" messages.
> One more thing: when using multiple workers and the asynchronous model, the
> error may not be related to the last printed method name:
>
> ```
> Handling json-rpc method A
> Handling json-rpc method B
> Some error related to the handling of request A
> ```
>
> If you just see the error like this, you would think it's cause by request B,
> even though it's when handling request A. Passing some context to the
> callback would allow us to print the request which is really at the origin of
> the error, avoiding any confusion. And it would allow us to not print it if
> everything goes fine.
clangd logs were never designed to fire only on errors. I probably misread the
patch initially, are you trying to change `log` into something used only for
logging errors?
Could you give a bit more context for your use-case in order to understand it
better?
- How do you use the log output? Is it shown to the end user?
- What information do you want to extract from the logs apart from the fact
that an error happened?
- Why are JSON RPC error responses not enough to indicate errors? What benefits
do logs provide over them?
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D44226
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
