Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang-tools-extra/clangd/ClangdServer.h | ||
---|---|---|
195–198 ↗ | (On Diff #329722) | This overload is now only used for tests, should it be removed and update all the tests that use it to the new version. |
clang-tools-extra/clangd/DraftStore.h | ||
25–28 | This is here for convenience, but should probably go into another header, just unsure if any of them fit the purpose. |
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)
clang-tools-extra/clangd/ClangdServer.h | ||
---|---|---|
190 ↗ | (On Diff #329722) | 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) |
clang-tools-extra/clangd/Preamble.cpp | ||
230 | I don't think this change makes sense, we don't need ownership here | |
322 | again, no need for ownership | |
375 | and here |
I've cleaned some changes up a bit. I do agree its not as important as the dirty buffer case, but this is about uniformity as much as anything.
you're moving into a stringref here