[PATCH] D49267: [clangd] Watch for changes in compile_commands.json

[Ilya Biryukov via Phabricator via cfe-commits]

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