[PATCH] D54952: [clangd] DO NOT SUBMIT. Draft interfaces for build system integration.

Manuel Klimek via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Thu Nov 29 06:32:48 PST 2018


klimek added inline comments.


================
Comment at: clangd/BuildSystem.h:29
+/// Default compilation database used by clangd, based on the build system.
+class Integration : public GlobalCompilationDatabase {
+public:
----------------
ilya-biryukov wrote:
> klimek wrote:
> > 'Integration' is a bit of a weird name, as it doesn't tell me what it does.
> > What I'd have expected is an abstraction like 'Workspace' (which I believe is a domain term in IDEs) that would:
> > -> be in some form coupled to 1 build system
> > -> provide a file system view on the workspace
> > -> provide the ability to watch for changed files
> > 
> > Then the BuildSystem would:
> > -> provide the ability to watch for changed inputs / commands
> > -> provide the ability to "prepare all inputs"
> > -> potentially provide the ability to get a build graph
> > 
> > If Integration wants to be that, it should:
> > - not extend GlobalCompilationDatabase
> > - not have getCompileCommand itself
> > 'Integration' is a bit of a weird name, as it doesn't tell me what it does.
> It is, sorry about that. This is not final. I intended to put this into the 'buildsystem' namespace, so it'd be used as 'buildsystem::Integration', which would make more sense.
> 
> > What I'd have expected is an abstraction like 'Workspace' (which I believe is a domain term in IDEs) that would:
> > -> be in some form coupled to 1 build system
> > -> provide a file system view on the workspace
> > -> provide the ability to watch for changed files
> The abstraction you're looking for is called `BuildSystem` in this patch.
> It is decoupled from watching for files (this is handled by FileSystemProvider), it can watch files internally but tells the users about the changes in terms of source files that had their inputs changed.
> 
> > Then the BuildSystem would:
> > -> provide the ability to watch for changed inputs / commands
> > -> provide the ability to "prepare all inputs"
> This is exactly what `BuildSystem` does.
> 
> > -> potentially provide the ability to get a build graph
> I was thinking about this as well, but still not sure if this information is better inferred by clangd when running the compiler, so decided to leave it out until we have a use-case for that.
> 
> 
> > If Integration wants to be that, it should:
> > not extend GlobalCompilationDatabase
> > not have getCompileCommand itself
> There is a discrepancy between the needs of the build system implementations and what clangd has at the protocol level.
> - At the protocol level, clangd operates on the files and it needs to provide features on a per-file manner: get diagnostics for a file, run code completion in a file, format a file, etc.
> - At the build system implementation level, we care about different things: discovering which build system is used, watching for changes to important files (`BUILD`, `CMakeLists.txt`, `compiler_commands.json`, etc.), reporting those changes to the build system users, building the generated files, providing compile commands, adding build depenndecies, etc.
> 
> `buildsystem::Integration` serves as a layer that ties those two together and provides an interface that higher layers of clangd are more comfortable with, e.g. a file-based interface that `GlobalCompilatioDatabase` provides.
> 
> My initial goal was to **replace** the `GlobalCompilationDatabase` with `Integration` and make it a non-abstract class, making it clear it just wraps build system integration into a form that's a bit more convenient for clangd uses. This turned out to be some work (we have `OverlayCDB` and test compilation databases that rely on this interface, so it's some work and it's not clear if that would make the code simpler at the end, so I decided to postpone it for now.
> 
So, I would still vote for separating a Workspace from a BuildSystem, as I'd expect a Workspace over time to grow VCS capabilities, and I'd expect having one abstractions that covers both builds and VCSs confusing.

I now understand the "Integration" layer better, but it still looks like it's doing too many things:
specificially, it seems like it's basically at the same time a single view of all build systems / workspaces AND the thing that manages them - and I'd definitely split that up; that is, both of these are useful but they should be different classes.


Repository:
  rCTE Clang Tools Extra

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54952/new/

https://reviews.llvm.org/D54952





More information about the cfe-commits mailing list