njames93 accepted this revision.
njames93 added a comment.
This revision is now accepted and ready to land.
LGTM, with a couple of nits that could easily be disregarded.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:84-105
+ for (auto I = path::begin(Parent, path::Style::posix),
+ E = path::end(Parent);
+ I != E; ++I) {
+ assert(I->end() < Parent.begin() && I->end() > Parent.end() &&
+ "Canonical path components should be substrings");
+ Ancestors.emplace_back(Parent.begin(), I->end() - Parent.begin());
+ }
----------------
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.
================
Comment at: clang-tools-extra/clangd/TidyProvider.cpp:61-62
+ const ThreadsafeFS &FS;
+ std::string RelPath;
+ std::chrono::steady_clock::duration MaxStaleness;
----------------
sammccall wrote:
> 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...)
nit: Unless we actually do extract this as a base class, these don't really
need to be fields.
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
