kadircet added a comment.
thanks looks really cool!
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:39
+ std::vector<CompiledFragment> &Out) {
+ assert(llvm::sys::path::is_absolute(Path));
+ auto FS = TFS.view(/*CWD=*/llvm::None);
----------------
I believe this function is going to be hot, I would suggest putting a span here
to track the latencies
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:41
+ auto FS = TFS.view(/*CWD=*/llvm::None);
+ auto Stat = FS->status(Path);
+ if (!Stat || !Stat->isRegularFile()) {
----------------
i suppose doing this IO on most of our features are OK, as they tend to search
for .clang-format at least (even the ancestor traversal), but I wonder whether
these will also be triggered for code completion and signature help, as they
currently don't do any IO (apart from preamble deserialization, and maybe one
patched include file).
Not a concern for this patch, but something to keep in mind when integrating
with the rest.
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:57
+ // For simplicity, don't cache the value in this case (use a bad key).
+ if (Buf->get()->getBufferSize() != Size) {
+ Size = -1;
----------------
is this really necessary ? we are setting the cache key to old stat values
anyway, so the next read shouldn't match neither the Size nor MTime.
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:67
+ }
+ llvm::copy(CachedValue, std::back_inserter(Out));
+ }
----------------
nit: I've found the nestedness a bit overwhelming, wdyt about putting this into
a scope_exit function and using early returns above ?
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:120
+ I != E; ++I) {
+ // Avoid weird non-substring cases like phantom "." components.
+ if (I->end() < Parent.begin() && I->end() > Parent.end())
----------------
ummm, what ? is this a bug in path iterator that should be fixed rather than
worked around?
================
Comment at: clang-tools-extra/clangd/ConfigProvider.cpp:181
+ for (const auto &Fragment : getFragments(P, DC))
+ Fragment(P, C);
+ return C;
----------------
nit: I think this looks a bit ... surprising
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:72
+
+ /// A provider that includes fragments from all the supplied providers.
+ static std::unique_ptr<Provider>
----------------
maybe also mention precedence order ? I suppose it already has the natural, the
latter one will override the former behavior, but saying out explicitly
wouldn't hurt.
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:74
+ static std::unique_ptr<Provider>
+ combine(std::vector<std::unique_ptr<Provider>>);
+
----------------
maybe `chain` instead of `combine` ?
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:85
+ /// When parsing/compiling, the DiagnosticCallback is used to report errors.
+ /// Usual thread-safety guarantees apply: this function must be threadsafe.
+ virtual std::vector<CompiledFragment>
----------------
first part of this sentence doesn't say much, maybe drop it?
================
Comment at: clang-tools-extra/clangd/ConfigProvider.h:91
+/// A provider that reads config from a YAML file in ~/.config/clangd.
+std::unique_ptr<Provider> createFileConfigProvider(const ThreadsafeFS &);
+
----------------
doesn't seem to be implemented anywhere? I suppose it is just a convenience
wrapper around `Provider::fromYAMLFile`, is it worth it?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D82964/new/
https://reviews.llvm.org/D82964
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
