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 {
----------------
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.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+ const ThreadsafeFS &FS;
+ std::string RelPath;
+ std::chrono::steady_clock::duration MaxStaleness;
----------------
Should these both be const?
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:76
+ namespace path = llvm::sys::path;
+ assert(llvm::sys::path::is_absolute(AbsPath));
----------------
Given path has been brought into scope in the line above can probably remove
excess qualifiers.
================
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;
----------------
How does this work with `..` components. Or does clangd ensure all absolute
paths have those removed?
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:113
+ OptionStack.push_back(std::move(*Config));
+ if (!OptionStack.back().InheritParentConfig)
+ break;
----------------
This will result in incorrect behaviour if a config specifies
`InheritParentConfig` as `false`
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
