ilya-biryukov accepted this revision.
ilya-biryukov added a comment.
This revision is now accepted and ready to land.
LGTM
================
Comment at: clangd/ClangdLSPServer.cpp:670
+ /*Output=*/"");
+ if (Old != New)
+ CDB->setCompileCommand(File, std::move(New));
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > Is this an optimization to not trigger compile command changes?
> > Can we perform it on the CDB level to make sure we hit it in the future if
> > more code calling `setCompileCommand` is added?
> > Is this an optimization to not trigger compile command changes?
> This is my attempt to preserve the optimization from rL338597, without having
> to keep the more complex interface to setCompileCommand.
> (And fix a bug in it: if there was an old command and you change it, you
> should refresh).
>
> The optimisation seems kind of suspect to me to be honest (if it's worth
> doing, it's probably worth doing right - i.e. per-file, not globally), but
> it's easy enough to preserve for now.
>
> > Can we perform it on the CDB level to make sure we hit it in the future if
> > more code calling setCompileCommand is added?
> Not quite sure what you mean: ClangdLSPServer needs to check whether there
> were changes in order to decide whether to invalidate. Having
> CompilationDatabase *also* aware of changes seems more complex/coupled and no
> less error-prone.
Reparsing the files if compile commands did not change is (almost) a no-op now,
so not sure it is worth it either.
SG to keep it for now, though.
> Not quite sure what you mean: ClangdLSPServer needs to check whether there
> were changes in order to decide whether to invalidate. Having
> CompilationDatabase *also* aware of changes seems more complex/coupled and no
> less error-prone.
I initially thought this patch also adds watches for changes to compile
commands (we discussed those yesterday), but the assumption was wrong, please
ignore the comment.
================
Comment at: clangd/GlobalCompilationDatabase.h:79
+/// using an in-memory mapping.
+class OverlayCDB : public GlobalCompilationDatabase {
public:
----------------
sammccall wrote:
> ilya-biryukov wrote:
> > The name does not seem to fully capture what this class does, i.e. allowing
> > to override compile commands for some files.
> > Maybe use `OverridenCDB` or something similar?
> Hmm, I feel the opposite way about these names - overlaying is exactly
> selective overriding of some elements, and overriding is more vague.
>
> I did try to think of some other names, I almost like "mutable" but it's not
> actually essential, e.g. we don't allow the fallback flags to be mutated.
Well, the comment is there so it's probably fine either way.
My reasoning behind "Overlay" is that it's usually a thing that combines two or
more "layers", calling one after another, without introducing any extra
functionality on its own.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D53687
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
