ilya-biryukov added a comment.
Sorry for the delays. Not sure about emitting `Queued` on the main thread, see
the corresponding comment. Otherwise looks very good.
================
Comment at: clangd/ClangdServer.h:49
+ /// Called whenever the file status is updated.
+ virtual void onFileUpdated(PathRef File, const TUStatus &Status){};
};
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > Have we thought about the way we might expose statuses via the LSP?
> > > > The `TUStatus` seems a bit too clangd-specific (i.e.
> > > > `BuildingPreamble`, `ReuseAST` is not something that makes sense in the
> > > > protocol). Which might be fine on the `ClangdServer` layer, but it
> > > > feels like we should generalize before sending it over via the LSP
> > > > The TUStatus seems a bit too clangd-specific (i.e. BuildingPreamble,
> > > > ReuseAST is not something that makes sense in the protocol).
> > >
> > > Yes, this is by design. `TUStatus` presents the internal states of
> > > clangd, and would not be exposed via LSP.
> > >
> > > Some ways of exposing status via the LSP
> > > - define a separate structure e.g. `FileStatus` in LSP, render
> > > `TUStatus` to it, and sending it to clients (probably we might want to
> > > add a new extension `textDocument/filestatus`)
> > > - reuse some existing LSP methods (`window/showMessage`,
> > > `window/logMessage`), render `TUStatus` to one of these methods' params.
> > LG, I believe the `BuildingPreamble` status in particular might be useful
> > on the C++ API level for us.
> > Not sure `showMessage` or `logMessage` is a good idea, the first one will
> > definitely be annoying if it'll be popping up all the time. Having a new
> > method in the LSP LG.
> `showMessage` seems work well via vim YCM, but it is annoying in vscode (as
> it pops up diags) -- we need to customize this behavior in our own extensions
> (showing messages in status bar). We need to revisit different ways.
I'd err on the side of getting a new LSP method/extension that's supposed to
show messages in the status bar.
It seems `showMessage` was specifically designed for user interactions and not
status bar updates. But that's out of the scope of this patch, so definitely
not blocking it.
================
Comment at: clangd/TUScheduler.cpp:268
+ /// Status of the TU.
+ TUStatus Status; /* GUARDED_BY(DiagMu) */
};
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Is `Status` actually ever read from multiple threads?
> > I assume it's only accessed on the worker thread, right? I think we can
> > leave out the locks around it and put beside other fields only accessed by
> > the worker thread, i.e. `DiagsWereReported`, etc.
> >
> > PS Sorry go going back and forth, I didn't think about it in the previous
> > review iteration.
> Unfortunately not, most statuses are emitted via worker thread, but we emit
> `Queued` status in the main thread...
Hm, that sounds surprising.
What if the rebuild is in progress on the worker thread and we emit the queued
status on the main thread at the same time? We might get weird sequences of
statuses, right?
E.g. `Queued → BuildingPreamble → [new update on the main thread] Queued →
[processing request on a worker thread] RunningAction('XRefs')`
The `Queued` request status between `BuildingPreamble` and `RunningAction`
looks **really** weird.
Maybe we could avoid that by setting emitting the `Queued` status on the worker
thread whenever we're about to sleep on the debounce timeout?
================
Comment at: clangd/TUScheduler.cpp:636
+ if (Requests.empty())
+ emitTUStatus({TUAction::Idle, /*Name*/ ""});
}
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe do it outside the lock? The less dependencies between the locks we
> > have, the better and this one seems unnecessary.
> Is it safe to read the `Requests` out of the lock here?
No, but we could store the value of `.empty()` in a local var under the lock.
The code looks a bit weirder, but that's a small price to pay for less locks
being held at the same time.
================
Comment at: clangd/TUScheduler.h:57
+ enum State {
+ Unknown,
+ Queued, // The TU is pending in the thread task queue to be
built.
----------------
hokein wrote:
> ilya-biryukov wrote:
> > Maybe remove this value and leave out the default constructor too?
> > That would ensure we never fill have `State` field with undefined value
> > without having this enumerator.
> > And FWIW, putting **any** value as the default seems fine even if we want
> > to keep the default ctor, all the users have to set the State anyway.
> We need the default constructor... When ASTWorker is constructed, we don't
> know the any state, I'm nervous about putting **any value** as default value
> (it will hurt the code readability, future readers might be confused -- why
> the status is `Queued` when constructing).
>
> Adding an `Unknown` seems ok to me, it is a common practice...
`Idle` would be the valid state for an `ASTWorker` with an empty queue. Why not
use it instead?
Can't disagree using `Unknown` is a common practice, but is it a good one? It
forces every caller to handle this value, even though it's semantics are always
unclear.
Why not use `Optional<EnumWithoutUnkown>` if I really want to indicate the lack
of value in those cases? Otherwise every `enum` type implements its own version
of `Optional`.
================
Comment at: clangd/TUScheduler.h:60
+ BuildingPreamble, // The preamble of the TU is being built.
+ BuildingFile, // The TU is being built.
+ Idle, // Indicates the worker thread is idle, and ready to run any upcoming
----------------
hokein wrote:
> ilya-biryukov wrote:
> > hokein wrote:
> > > ilya-biryukov wrote:
> > > > What's the fundamental difference between `BuildingFile` and
> > > > `RunningAction`?
> > > > We will often rebuild ASTs while running various actions that read the
> > > > preamble.
> > > >
> > > > Maybe we could squash those two together? One can view diagnostics as
> > > > an action on the AST, similar to a direct LSP request like
> > > > findReferences.
> > > They are two different states, you can think `RunningAction` means the
> > > AST worker starts processing a task (for example `Update`,
> > > `GoToDefinition`) and `BuildingFile` means building the AST (which is one
> > > possible step when processing the task).
> > >
> > > In the current implementation, `BuildingPreamble` and `BuildingFile` are
> > > only emitted when AST worker processes the `Update` task (as we are
> > > mostly interested in TU states of `ASTWorker::update`); for other AST
> > > tasks, we just emit `RunningAction` which should be enough.
> > >
> > > Given the following requests in the worker queue:
> > > `[Update]`
> > > `[GoToDef]`
> > > `[Update]`
> > > `[CodeComplete]`
> > >
> > > statuses we emitted are
> > >
> > > `RunningAction(Update) BuildingPreamble BuildingFile`
> > > `RunningAction(GoToDef)`
> > > `RunningAction(Update) BuildingPreamble BuildingFile`
> > > `RunningAction(GetCurrentPreamble)`
> > > `Idle`
> > >
> > That's the confusing part: clangd might be building the ASTs inside
> > `RunningAction` too, but we only choose to report `BuildingFile` during
> > updates.
> > I would get rid of `BuildingFile` and change it to
> > `RunningAction(Diagnostics)` instead, it feels like that would allow
> > avoiding some confusion in the future.
> >
> `BuildingFile` might be more natural (understandable) than `Diagnostics`. Two
> possible ways here:
>
> - we can also report `BuildingFile` for other AST tasks, does it reduce the
> confusing part?
> - or if you think `Diagnostic` is an important state of `Update`, how about
> adding a new state `RunningDiagnostic`? `RunningAction` is a higher level
> information IMO, clangd is running diagnostic inside `RunningAction(Update).`
>
> WDYT?
If you look at the `TUScheduler` as a black box, diagnostics are not that much
different from other actions reading the AST.
I'd err on the side of not sending updates unless something interesting is
actually going on.
What we're currently doing makes sense (sending a single update per action), I
think it doesn't really matter if we call it `BuildingFile` or
`RunningAction("Diagnostics")` or `RunningDiagnostics`. Pick the one you like.
Let's put a comment on `BuildingFile` that it's only emitted when building the
AST for diagnostics and the read actions emit `RunningAction` instead
Repository:
rCTE Clang Tools Extra
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D54796/new/
https://reviews.llvm.org/D54796
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
