The plan is to use this to use this for .clang-format, .clang-tidy, and
compile_commands.json. (Currently the former two are reparsed every
time, and the latter is cached forever and changes are never seen).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
To give a bit of context here: the end-goal here is hot-reload of compile_commands.json. (This covers our *two* top bugs: #192 and #83).
I'm very happy with the hot-reloading of config files (.clangd etc) and I think it's a good fit to generalize.
In particular, config parsing shouldn't be repeated unneccesarily as it has side-effects (diagnostics). Similarly, compile_commands parsing shouldn't be repeated as it's expensive (the file is often huge).
So this is better than other approaches like caching at the FS level.
We could reasonably apply this to .clang-format and .clang-tidy as well. Currently we stat up the directory chain, and then read and parse these every time. It's not very expensive as these are small files, but it's also not free.
In all of these cases, the costs are going to be in eschewing the simple APIs that hide all the IO. For compile_commands.json, we need to extend or stop using the generic compilation-database plugin mechanism. For clang-tidy/format we need to introduce a tree-shaped cache (we have one for CDBs already).
Yes, I was looking at copying the config cache for use with .clang-tidy
clang-tools-extra/clangd/support/FileCache.h | ||
---|---|---|
43 | nit: Is it more descriptive to have this spelled as PathRef, same for path() below. |
Sorry for such a tremendous delay.
Mostly looks good functionally, I just have a few questions about semantics and several stylistic nits.
Also, thanks for the amount of documentation - it makes it really nice to navigate around and review.
clang-tools-extra/clangd/support/FileCache.cpp | ||
---|---|---|
18 | Uhhh. Doesn't Clang-Tidy think this is a bad idea? IIUC this is not UB but it looks quite scary regardless. Maybe use std::numeric_limits<uint64_t>::max() here? | |
44 | Wait, I thought ValidTime is the last time point when the file could be correctly parsed (i.e. the file was valid). But if the parsing could not be done then this would be bumped regardless, right? So what's the "validity" semantics here? | |
clang-tools-extra/clangd/support/FileCache.h | ||
23 | s/live/fresh? | |
58 | So the client is responsible for keeping FreshTime updated, right? I think it might be worth mentioning in the comments. | |
70 | I assume MTime is ModifiedTime - took me some time to realize it (getting into the code), maybe expand or add some comment. Also, why are the types different with ValidTime? | |
clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp | ||
15 | Please sort the includes here (clangd should have a Code Action here). |
clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp | ||
---|---|---|
15 | Ahh, I didn't know that and I had some of the valid warnings in my patches. I see, thanks! |
clang-tools-extra/clangd/support/FileCache.cpp | ||
---|---|---|
18 | Done. | |
44 | Ah, "valid" refers to the cache, not the file. The cache is valid if it represents what's on disk, whether that's good, bad or missing. (Note that this class doesn't actually know anything about the semantics of the file content, or whether it's valid). Added a comment to the member. | |
clang-tools-extra/clangd/support/FileCache.h | ||
23 | I guess it depends which way you look at it: | |
70 | Renamed to ModifiedTime. (mtime is a term of art: see mtime/ctime/atime. But spelling it out is nearly as precise and less cryptic). The types are different because they are different coordinate systems: steady_clock is approximately "nanos since boot" which is the best way to compare timestamps captured within a program, and system_clock has semantics that match mtime stored in the filesystem (though actually we just compare for equality, so it's just the most convenient type). Changed ModifiedTime to spell the underlying std type directly to make this difference clearer (though the API we get it from uses the LLVM typedef). |
clang-tools-extra/clangd/support/FileCache.h | ||
---|---|---|
58 | I'm not really following - FreshTime is a parameter and must be provided each time, so I'm not sure what "keep it updated" means. It's true that clients likely want to pass now() + some_duration, I've added an example explaining that to the docs. | |
clang-tools-extra/clangd/unittests/support/FileCacheTests.cpp | ||
15 | I can apply the fix, but then arc diff wants to revert it. I don't think this is important enough to fight the tools over. |
clang-tools-extra/clangd/support/FileCache.cpp | ||
---|---|---|
60 | Is tracking the size really that important? |
clang-tools-extra/clangd/support/FileCache.cpp | ||
---|---|---|
60 | If the content changes without changing mtime, and we don't detect it using size, we're stuck with an invalid cache until it changes again (which could be forever).
My best guess is if the FS has a low resolution (e.g. FAT32, though hopefully people aren't putting config files on USB sticks...) or the file was written twice e.g. by a script fast enough to beat the timer. In both cases I'd want the changes to be revealed.
Yup, I don't have a good answer for that case. Hopefully it's very rare. I don't think we should let the perfect be the enemy of the good. |
s/live/fresh?