This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Cache .clang-tidy files again.
ClosedPublic

Authored by sammccall on Nov 25 2020, 1:28 PM.

Details

Summary

This cache went away in 73fdd998701cce3aa6c4d8d2a73ab97351a0313b

This time, the cache is periodically validated against disk, so config
is still mostly "live".

The per-file cache reuses FileCache, but the tree-of-file-caches is
duplicated from ConfigProvider. .clangd, .clang-tidy, .clang-format, and
compile_commands.json all have this pattern, we should extract it at some point.
TODO for now though.

Diff Detail

Event Timeline

sammccall created this revision.Nov 25 2020, 1:28 PM
sammccall requested review of this revision.Nov 25 2020, 1:28 PM
njames93 added inline comments.Nov 25 2020, 3:08 PM
clang-tools-extra/clangd/TidyProvider.cpp
34

To save a copy, could this not return a const pointer to whats stored in Value, nullptr if Value is empty.
In DotClangTidyTree, OptionStack could then store pointers instead of values.
Considering the size of ClangTidyOptions (312 bytes in x64 with libstdc++) this is definitely a worthwhile saving.

One slight issue is I'm not sure how nicely this approach would play with the mutex.

64–65

Should these both be const?

79

Given path has been brought into scope in the line above can probably remove excess qualifiers.

87–90

How does this work with .. components. Or does clangd ensure all absolute paths have those removed?

116

This will result in incorrect behaviour if a config specifies InheritParentConfig as false

sammccall updated this revision to Diff 307924.Nov 26 2020, 1:57 PM
sammccall marked 3 inline comments as done.

Return shared_ptr instead of value from cache, to avoid copies.

Remove unneeded qualifiers.

clang-tools-extra/clangd/TidyProvider.cpp
34

You're right, and 312 bytes understates it - it contains strings and stuff that likely allocate on the heap when copied.

The issue with a pointer is that the underlying value may not live long enough (or to put it another way, the pointer will point to the cache slot which may be concurrently modified).

I think the fix for that is to store and return a shared_ptr<const ClangTidyOptions>. I'll give that a try.

64–65

We've generally avoided making members const in clangd when they can be, probably mostly to avoid problems with move semantics, but in the end because you've got to pick a style and stick with it.

(The move-semantics argument seems weak because we do use reference members sometimes, including in this class, and convert them to pointers when it becomes a problem. Oh well...)

87–90

Right, paths are always canonical.

116

Whoops, thanks!
(Why is this optional<bool> rather than bool?)

njames93 added inline comments.Nov 26 2020, 4:10 PM
clang-tools-extra/clangd/TidyProvider.cpp
34

That sounds like a good resolve. I'm guessing in the common case where the config file doesn't change, this would never mutate.

87–90

If that's the case, aren't these checks redundant. Maybe put some asserts in so we don't pay for them in release builds

116

I'm honestly not sure. Personally I don't think it should belong in ClangTidyOptions. Its only used when reading configuration from files(or command line). Maybe I'll rustle something up for that.

njames93 added inline comments.Nov 26 2020, 4:26 PM
clang-tools-extra/clangd/TidyProvider.cpp
116

What do you think about this approach? D92199

sammccall updated this revision to Diff 308004.Nov 27 2020, 3:10 AM
sammccall marked 3 inline comments as done.

Replace dynamic check with assert.

njames93 accepted this revision.Nov 27 2020, 5:10 AM

LGTM, with a couple of nits that could easily be disregarded.

clang-tools-extra/clangd/TidyProvider.cpp
64–65

nit: Unless we actually do extract this as a base class, these don't really need to be fields.

84–105

nit: Is it not better to simply merge these 2 loops into one traversal and get rid of the Ancestors vector. The path traversal is relatively cheap so we wouldn't be holding the mutex for that much longer.

This revision is now accepted and ready to land.Nov 27 2020, 5:10 AM
This revision was landed with ongoing or failed builds.Nov 29 2020, 4:29 AM
This revision was automatically updated to reflect the committed changes.
sammccall marked an inline comment as done.

Unfortunately this broke the search order for clang-tidy files in root directories in favour or subdirectories. This wasn't caught as we didn't have tests for this. I've put a fix for this in D96204 with appropriate tests.