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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
