njames93 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 {
----------------
sammccall wrote:
> 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.
That sounds like a good resolve. I'm guessing in the common case where the
config file doesn't change, this would never mutate.
================
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;
----------------
sammccall wrote:
> njames93 wrote:
> > How does this work with `..` components. Or does clangd ensure all absolute
> > paths have those removed?
> Right, paths are always canonical.
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
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+ OptionStack.push_back(std::move(*Config));
+ if (!OptionStack.back().InheritParentConfig)
+ break;
----------------
sammccall wrote:
> 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?)
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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D92133/new/
https://reviews.llvm.org/D92133
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
