sammccall added inline comments.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:34
+ llvm::Optional<tidy::ClangTidyOptions>
+ get(const ThreadsafeFS &TFS,
+ std::chrono::steady_clock::time_point FreshTime) const {
----------------
njames93 wrote:
> 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.
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.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+ const ThreadsafeFS &FS;
+ std::string RelPath;
+ std::chrono::steady_clock::duration MaxStaleness;
----------------
njames93 wrote:
> Should these both be const?
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...)
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-87
+ // Avoid weird non-substring cases like phantom "." components.
+ // In practice, Component is a substring for all "normal" ancestors.
+ if (I->end() < Parent.begin() && I->end() > Parent.end())
+ continue;
----------------
njames93 wrote:
> How does this work with `..` components. Or does clangd ensure all absolute
> paths have those removed?
Right, paths are always canonical.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+ OptionStack.push_back(std::move(*Config));
+ if (!OptionStack.back().InheritParentConfig)
+ break;
----------------
njames93 wrote:
> This will result in incorrect behaviour if a config specifies
> `InheritParentConfig` as `false`
Whoops, thanks!
(Why is this optional<bool> rather than bool?)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92133/new/
https://reviews.llvm.org/D92133
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
