This is an archive of the discontinued LLVM Phabricator instance.

[ClangD] Refactor clangd into separate components
ClosedPublic

Authored by ilya-biryukov on May 10 2017, 8:56 AM.

Details

Event Timeline

ilya-biryukov created this revision.May 10 2017, 8:56 AM
ilya-biryukov added a project: Restricted Project.May 11 2017, 1:14 AM
ilya-biryukov added a subscriber: cfe-commits.
klimek added inline comments.May 11 2017, 2:11 AM
clangd/ClangdLSPServer.h
23

I'd have expected something that's called LSP server to work on the LSP protocol level (that is, have a server(iostream) equivalent method).

24–25

I like to put the public interface first, as that's what people usually look at first.

48

AFAIK usually we don't duplicate access modifiers.

clangd/ClangdUnit.h
44

One question is how we can handle when we want the old one to stick around while we do a full reparse (when #includes change), so we still have fast code completion.

clangd/ClangdUnitStore.h
21

Tiny nit: please add empty lines between classes and namespace delcs :)

29–30

Not having read enough of the rest of the patch yet, I'd expect these things to be stored on the class level instead of being passed around all the time.
Similarly, we're missing the VFS?

31

Parameters after a function are unfortunate, as they make the code flow weird when you want to pass in a lambda, but still want to set the parameter after it :(
Given how different the semantics are, I'd perhaps make these 2 functions:

// Run on the translation unit.
// If there was a previous call of runOnUnitWithContent, the content provided there will be used.
// Otherwise, the file will be read from VFS.
runOnStoredUnit(PathRef File, Func Action);

// Run on the translation unit with the given content.
// Content will be stored as in-flight code for the given translation unit only:
// - if a different translation unit is parsed that #includes File, Content will not be used;
// - subsequent calls to runOnStoredUnit will re-use Content.
runOnUnitWithContent(PathRef File, StringRef Content, Func Action);
32

This is due to a restriction in the AST unit? If we don't need to reparse, why can't we run multiple things over an AST at once?

clangd/GlobalCompilationDatabase.cpp
55

Here and elsewhere: in Clang, we use FIXME instead of TODO :)

clangd/GlobalCompilationDatabase.h
28

What's global about this?

ilya-biryukov marked 3 inline comments as done.

Addressed review comments

ilya-biryukov added inline comments.May 11 2017, 3:47 AM
clangd/ClangdLSPServer.h
23

The idea is to have a class that wraps ClangdServer and is called by clangd's ProtocolHandlers.cpp to do all the work.
I.e. it's a replacement of what used to be called 'ASTManager'. Any suggestions for a better name? ClangdLSPHandler?

It's seems hard to implement the interface you suggest right away, lots of code will have to be moved. Happy to do that in further commits, but suggest we leave this class as is in this commit.

clangd/ClangdUnit.h
44

My idea was to have two ClangdUnits for that and handle that somewhere else(i.e. ClangdUnitStore seem like a good place to do that)

clangd/ClangdUnitStore.h
29–30

The VFS changes are not in this commit, they will follow later, that's an initial refactoring of clangd to allow more gradual changes later.

The idea is that this class is an implementation detail of ClangdServer that handles synchronized access to underlying ClangdUnits(which are not thread-safe) and doesn't do anything else (i.e. doesn't care about storing the contents of the files etc).

31

Moved the Func parameter to the end of the list.

The difference is semantics is different from what it seemed. Added comments and split function into two to clarify that.
We need Content in both cases, because if there's no ClangdUnit, then we'll have to create it and it requires initial text. And most actions need to call 'ClangdUnit::reparse' before they are run. But not code completion, which basically does reparse itself, hence the ReparseBeforeActionParameter.
See updated code for more details.

32

There are things like codeComplete that are actually mutable, but all other things could probably be run in parallel. (Don't have a great insight into that, though).
I really didn't want to change any of clangd behaviour in that commit, but I'd be glad to try adding that later.
What we could have in the future:
runOnUnitUnderReadLock(... [](const ClangdUnit &Unit) { auto diags = Unit.getLocalDiagnostics(); /* ... */} );
runOnUnitUnderWriteLock(... []( ClangdUnit &Unit) { auto completions = Unit.codeComplete(); /* ... */} );

clangd/GlobalCompilationDatabase.h
28

The fact that it's created only once(as opposed to tooling::CompilationDatabase, which can be recreated for each file) and handles all CompileCommand-related things(i.e. the idea is to add tracking of compile flags changes to this interface).
I called it ClangdCompilationDatabase first, but then I thought there's too much Clangd-prefixed names :-)
Any suggestions for a better name?

krasimir added inline comments.May 11 2017, 6:44 AM
clangd/CMakeLists.txt
12

nit: we might want to keep these sorted

clangd/ClangdMain.cpp
41

The combination of the name and the type of this variable makes no sense.

clangd/ClangdServer.h
41–43

I feel that shouldBuildDiagnosticsForFile doesn't belong to this interface.

49

If this models a request to the worker thread, why not call it WorkerRequest or something? The current name is confusing, because it implies that also other requests might be handled in this way.

62

Maybe explicitly mention Worker in the name of this class?

68

The constructor already gets a ClangdServer, why pass it here?

ilya-biryukov marked 3 inline comments as done.

Addressed style comments.
Moved ClangdScheduler to be the last field of the ClangdServer, so
that it's constructed last and destructed first.

ilya-biryukov added inline comments.May 11 2017, 8:31 AM
clangd/ClangdMain.cpp
41

Totally agree, renamed it.

clangd/ClangdServer.h
41–43

Sorry, this sneaked in from some code that didn't end up in this commit.
It's also not used anywhere now. I've removed it from this change.

62

I actually like the name ClangdScheduler. Albeit, it's a bit too generic and lives inside the clangd namespace, so I'm afraid of clashes later on, but until that happens I'd rather keep it.
Do you find ClangdScheduler confusing? Does the comment help?
But if you feel stongly that we shall change the name, let's do it.

68

Just to avoid introducing redundant fields.
(I.e. ClangdServer stores ClangdScheduler, which stores a reference to ClangdServer).

klimek added inline comments.May 12 2017, 2:25 AM
clangd/ClangdLSPServer.h
23

I think it's fine to have an intermediate step checked in; the important part is that we add comments explaining why this is the way it is, especially if we plan to change it in the future :) -> I'd add a class level comment explaining what this class does and why it exists.

clangd/ClangdUnit.h
44

Tiny nit: generally, use "." at the end of sentences :)

clangd/GlobalCompilationDatabase.h
28

Why don't we just use a tooling::CompilationDatabase and pass it around?

ilya-biryukov marked an inline comment as done.

Addressed new comments

clangd/ClangdLSPServer.h
23

Added comments to this class. And ClangdServer, it didn't have a comment too.

clangd/ClangdUnit.h
44

Fixed

clangd/GlobalCompilationDatabase.h
28

It has getAllFiles and getAllCompileCommands which we don't need for now.
It doesn't have capabilities to track changes to flags, which we'll want to add at some point. (Added a comment that we'll be adding those).

Also, currently implementations of CompilationDatabase actually require us to recreate them for each file, this logic is currently moved into DirectoryBasedGlobalCompilationDatabase for convenience.

bkramer accepted this revision.May 15 2017, 6:43 AM

I believe this is good enough now.

This revision is now accepted and ready to land.May 15 2017, 6:43 AM
ilya-biryukov closed this revision.May 15 2017, 7:30 AM
ilya-biryukov marked an inline comment as done.