ilya-biryukov 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:
----------------
klimek wrote:
> 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.
> 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.
That makes sense to me too, but I don't have the full understanding of the VCS
capabilities we might want to have. Something that popped up before in various
discussions was tracking a changing VCS branch to keep separate versions of the
index to hotswap. Will have to think more about it, thanks for the input.
> 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.
That makes total sense, the implementation is actually split even though it's
not visible in this patch: the private `Discovery` class handles the loading of
the build system tree, etc.
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
