sammccall added a comment.
Avoiding copies seems nice, but this makes some interfaces more awkward. Do you
have any measurements that some of these copies matter?
(The dirty FS changes avoided coping all the dirty buffers at once, but it
seems like these changes will mostly save ~1 file copy per operation)
================
Comment at: clang-tools-extra/clangd/ClangdServer.h:190
/// diagnostics etc built from it. If empty, a version number is generated.
+ void addDocument(PathRef File, std::shared_ptr<const std::string> Contents,
+ llvm::StringRef Version = "null",
----------------
what's the purpose of changing this interface?
The only callers that matter are in ClangdLSPServer, and this signature
requires it to copy anyway. So why not copy inside ClangdServer?
(StringRef is more convenient to for tests and C++ embedders, and just a bit
less churn)
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:231
llvm::Expected<ScannedPreamble>
-scanPreamble(llvm::StringRef Contents, const tooling::CompileCommand &Cmd) {
+scanPreamble(std::shared_ptr<const std::string> Contents,
+ const tooling::CompileCommand &Cmd) {
----------------
I don't think this change makes sense, we don't need ownership here
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:324
// without those.
- auto ContentsBuffer =
- llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
+ auto ContentsBuffer = getSharedStringBuffer(Inputs.Contents, FileName);
auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
----------------
again, no need for ownership
================
Comment at: clang-tools-extra/clangd/Preamble.cpp:376
const CompilerInvocation &CI) {
- auto ContentsBuffer =
- llvm::MemoryBuffer::getMemBuffer(Inputs.Contents, FileName);
+ auto ContentsBuffer = getSharedStringBuffer(Inputs.Contents, FileName);
auto Bounds = ComputePreambleBounds(*CI.getLangOpts(), *ContentsBuffer, 0);
----------------
and here
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D98364/new/
https://reviews.llvm.org/D98364
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
