This is an archive of the discontinued LLVM Phabricator instance.

[clangd] allow clients to pass in compilationDatabaseChanges in the 'workspace/didChangeConfiguration' request
ClosedPublic

Authored by arphaman on Jul 24 2018, 1:59 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

arphaman created this revision.Jul 24 2018, 1:59 PM
simark added a subscriber: simark.Jul 24 2018, 2:10 PM

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

clangd/Protocol.h
429 ↗(On Diff #157121)
  • Maybe add a comment that the key of the map is a file name?
  • The value does not contain the working directory at the time, but we need that for building tooling::CompileCommand, maybe add it?
malaperle edited reviewers, added: malaperle; removed: malaperle-ericsson.Jul 26 2018, 10:25 AM

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

clangd/Protocol.h
429 ↗(On Diff #157121)

Could you please elaborate on the issue with the working directory? I didn't quite get that concern, sorry.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Maybe a cleaner design would be to untangle the two use-cases and control them with a flag to clangd?
I.e. we can have two implementations of compilation databases in clangd:

  • one that uses clang tooling capabilities, i.e. reads compile_commands.json, etc.
  • one that gets all compile commands from the protocol and won't use the clang tooling.

The command-line arg to clangd will control which implementation is used.

The advantage is that we don't have to think about interactions between the clang plugins and explicit overrides and it should be easier to make sure that we don't accidentally read compilation args from the wrong place.
Would also help to keep DirectoryBasedCompilationDatabase a bit simpler, and the other implementation would be extremely simple. WDYT?

ilya-biryukov added inline comments.Jul 27 2018, 6:30 AM
clangd/Protocol.h
429 ↗(On Diff #157121)

If arguments passed to clang contain relative paths, it's important to run compilation in the same working directory as the compiler.
E.g. clang++ -Ideps/boost/include foo.cpp

Even if that's a non-issue for your use-case (e.g. all paths are absolute or clangd runs in the same working dir as clang would), we should still allow passing custom working directories in this protocol extension.

The mode of operation where compile commands come from the client seems useful, but I wonder if there's any value in mixing it with compile_commands.json and other CDB plugins.
Do you plan to use the overridden commands in conjunction with CDB plugins or do you want the client to exclusively control the compile commands?

The client will control the commands exclusively.

Maybe a cleaner design would be to untangle the two use-cases and control them with a flag to clangd?
I.e. we can have two implementations of compilation databases in clangd:

  • one that uses clang tooling capabilities, i.e. reads compile_commands.json, etc.
  • one that gets all compile commands from the protocol and won't use the clang tooling.

The command-line arg to clangd will control which implementation is used.

The advantage is that we don't have to think about interactions between the clang plugins and explicit overrides and it should be easier to make sure that we don't accidentally read compilation args from the wrong place.
Would also help to keep DirectoryBasedCompilationDatabase a bit simpler, and the other implementation would be extremely simple. WDYT?

Sounds good to me. I'll update the patch to do that.
I'll add the flag as well. We won't be setting this flag directly though, our XPC protocol will assume it, but that's fine for us.

arphaman updated this revision to Diff 157762.Jul 27 2018, 2:06 PM
arphaman marked 2 inline comments as done.

Updated patch to address review comments:

  • The compilation database updated are used only when '-in-memory-compile-commands' flag is used.
  • It's now possible to set the working directory as well.
ilya-biryukov added inline comments.Jul 30 2018, 12:46 AM
clangd/ClangdLSPServer.h
106 ↗(On Diff #157762)

This code starts to be a little hard to follow. Could we extract it into an external class that encapsulates this logic? Something like:

class ClangdLSPServer {

private:
  class CompilationDB {
  public:
    static CompilationDB makeInMemory();
    stiatc CompilationDB makeDirectoryBased();
  
    void invalidate(PathRef File);
     /// Only valid for in-memory CDB, no-op and error log on DirectoryBasedCDB.
    void setCompileCommandsForFile(PathRef File);
    /// Only valid for directory-based CDB, no-op and error log on InMemoryCDB;
    void setExtraFlags(PathRef File);
    /// Returns a CDB that should be used to get compile commands for the current instance of ClangdLSPServer.
    GlobalCompilationDatabase& getCDB(); 
  
  private:
     // if IsDirectoryBased is true, an instance of InMemoryCDB.
     // If IsDirectoryBased is false, an instance of DirectoryBasedCDB.  unique_ptr<GlobalCompilationDatabase> CDB;
    unique_ptr<GlobalCompilationDatabase> CDB;
    // non-null only for directory-based CDB
    unique_ptr<CachingCompilationDatabase> CachingCDB;
    bool IsDirectoryBased;
  };
 
  CompilationDB CDB;
  // .....
}

We can static_cast to InMemoryCDB or DirectoryBasedCDB based on the IsDirectoryBased flag to implement all the operations we define in the helper class.

clangd/GlobalCompilationDatabase.h
136 ↗(On Diff #157762)

Maybe remove 'mutable' from Commands? We shouldn't need it.

136 ↗(On Diff #157762)

Maybe make the value type CompileCommand instead of Optional<CompileCommand>?
We seem to only use the optional when inserting values and we could easily rewrite that code to use map::insert().

clangd/Protocol.h
425 ↗(On Diff #157762)

Maybe use a shorter name, e.g. ClangdCompileCommand? This class looks *almost* like tooling::CompileCommand, so having a similar name seems reasonable.

clangd/tool/ClangdMain.cpp
170 ↗(On Diff #157762)

The important distinction seems to be where the compile commands come from, maybe make the name centered on the fact that compile commands are read from the protocol rather from the filesystem?

E.g. using named option values:
--compile_args_from={lsp|filesystem}
Or a boolean option:
--compile_args_from_lsp

WDYT?

171 ↗(On Diff #157762)

Maybe elaborate a bit more on this mode? E.g. clangd will get compile all commands from via LSP and won't look at the filesystem for 'compile_commands.json' files

arphaman updated this revision to Diff 158024.Jul 30 2018, 11:27 AM
arphaman marked 6 inline comments as done.

Address review comments

clangd/tool/ClangdMain.cpp
170 ↗(On Diff #157762)

Yeah, that looks better. I added this argument in the updated patch.

Just a few nits left.

clangd/ClangdLSPServer.cpp
422 ↗(On Diff #158024)

Other usages of FileName in tooling::CompileCommand seem to set this either to an absolute or a relative-to-cwd path to the file for the command.
Maybe put the whole filepath there too? (even though I don't think we particularly rely on this anywhere in clangd)

539 ↗(On Diff #158024)

Other methods already assume it's non-null for directory-based CDB, so maybe remove the check for it being non-null here or replace the check with an assert?

test/clangd/did-change-configuration-params.test
43 ↗(On Diff #158024)

Matching the stderr output seems fragile, but that's the only way to inspect the actual compile command that's used, so that's probably fine here.
However, the paths on windows will probably differ (notice the regexp matches for file uris), so we might need to do the regexp match here as well, e.g.Updating file {{.*}}foo.c with command ... to keep the Windows buildbots happy.

arphaman updated this revision to Diff 158376.Jul 31 2018, 1:37 PM
arphaman marked 3 inline comments as done.

Updated to address review comments.

test/clangd/did-change-configuration-params.test
43 ↗(On Diff #158024)

Good point.
I also realized that I have to make this a UNIX-specific test as the compilation database updates require a path, which will obviously be different on Windows.

ilya-biryukov accepted this revision.Aug 1 2018, 2:20 AM

Thanks! LGTM

This revision is now accepted and ready to land.Aug 1 2018, 2:20 AM
This revision was automatically updated to reflect the committed changes.