ilya-biryukov added a reviewer: ilya-biryukov.
ilya-biryukov added a comment.
================
Comment at: clangd/ClangdLSPServer.cpp:430
CDB.clear();
-
- reparseOpenedFiles();
+ compileCommandsChangePost(CCChangeData);
}
----------------
ilya-biryukov wrote:
> simark wrote:
> > malaperle wrote:
> > > ilya-biryukov wrote:
> > > > Maybe keep the old logic of reparsing all open files? This would make
> > > > the change way simpler and I don't think we need this extra complexity
> > > > in the long run, when we have better integration with the build system.
> > > >
> > > > ClangdServer will reuse the preamble if compile command didn't change
> > > > anyway, so reparse will be very fast and shouldn't be affected.
> > > > If the compile command does change, we'll retrigger the full rebuild.
> > > I think the change is not that complex but brings much added value. About
> > > the integration with the build system, there are many build systems out
> > > there so I don't think better integration will be useful in many
> > > scenarios (plain make, custom builds, etc). This solution is generic
> > > enough so that any build system that generates compile_commands.json will
> > > be supported in a pretty good way.
> > @malaperle also suggested an alternative way of doing it. Instead of
> > saving the the compile commands in a custom structure before clearing the
> > cache, we could save the compile flags in the `ParsedAST` object. When
> > some compile_commands.json changes, we can compare the new compile flags
> > with this saved copy. I think it would make the code a bit more
> > straightforward.
> > I think the change is not that complex but brings much added value.
> > @malaperle also suggested an alternative way of doing it. Instead of saving
> > the the compile commands in a custom structure before clearing the cache,
> > we could save the compile flags in the ParsedAST object. When some
> > compile_commands.json changes, we can compare the new compile flags with
> > this saved copy. I think it would make the code a bit more straightforward.
>
> The no-op rebuilds in case nothing changes is a good and long overdue
> optimization, and I agree that `ParsedAST` or `TUScheduler::update` is
> probably the right place to put it into. Any other benefits we get from
> snapshotting the compile commands here? Could we make this optimization in a
> separate change? It does not seem directly related to the file watching
> changes. Also happy to look at it, finding the right place to do it might
> involve digging through layers of classes that manage the AST.
>
> > About the integration with the build system, there are many build systems
> > out there so I don't think better integration will be useful in many
> > scenarios (plain make, custom builds, etc). This solution is generic enough
> > so that any build system that generates compile_commands.json will be
> > supported in a pretty good way.
> Just to clarify, the "better buildsystem integration" I refer to is not about
> enabling support for more build systems (though, that would be nice-to-have),
> it's about building abstractions that better support the current use-cases,
> i.e. making the connection between the CompiationDatabase and
> compiler_commands.json more explicit, allowing the track changes in the build
> system, etc. We think that file-watching will definitely be part of it, but
> we don't have full design yet.
>
> In any case, we do think that there are improvements to be made both in
> compile_commands.json and in our internal Bazel integration.
D49783 makes the rebuild noop if compile command and preamble deps don't
change, I think this should be good enough to keep `rebuildOpenedFiles` call
for now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D49267
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
