Page MenuHomePhabricator

[clangd] Use ref counted strings throughout for File Contents
AcceptedPublic

Authored by njames93 on Mar 10 2021, 11:41 AM.

Details

Diff Detail

Event Timeline

njames93 created this revision.Mar 10 2021, 11:41 AM
njames93 requested review of this revision.Mar 10 2021, 11:41 AM
njames93 added inline comments.Mar 10 2021, 11:44 AM
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

njames93 updated this revision to Diff 329933.Mar 11 2021, 5:25 AM
njames93 marked 4 inline comments as done.

Remove some unneeded changes.

njames93 updated this revision to Diff 329939.Mar 11 2021, 5:41 AM

Remove other unnecessary ownership.

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.

sammccall accepted this revision.Mar 24 2021, 6:33 AM
sammccall added inline comments.
clang-tools-extra/clangd/ClangdLSPServer.cpp
678

you're moving into a stringref here

clang-tools-extra/clangd/ClangdServer.cpp
214

nit: we generally prefer to qualify the types, can you revert here?

This revision is now accepted and ready to land.Mar 24 2021, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 24 2021, 6:33 AM