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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits