Page MenuHomePhabricator

[clangd] Replaced WorkerRequest with std::function...
ClosedPublic

Authored by ilya-biryukov on May 22 2017, 8:51 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.May 22 2017, 8:51 AM
ilya-biryukov retitled this revision from Replaced WorkerRequest with std::function... to [clangd] Replaced WorkerRequest with std::function....May 22 2017, 8:52 AM
ilya-biryukov added a project: Restricted Project.
krasimir added inline comments.May 23 2017, 2:48 AM
clangd/ClangdServer.cpp
85 ↗(On Diff #99773)

Why are we taking it from the front and not from the back? Maybe at least add a comment about this behavior.

102 ↗(On Diff #99773)

Why did this get out?

230 ↗(On Diff #99773)

Maybe an explicit flush and no block?

clangd/ClangdUnitStore.h
56 ↗(On Diff #99773)

Maybe make it less generic and put the implementation in the source file? It uses std::function anyways.

krasimir added inline comments.May 23 2017, 2:51 AM
clangd/ClangdUnitStore.h
56 ↗(On Diff #99773)

Sorry, I didn't see the surroundings. Disregard this comment.

62 ↗(On Diff #99773)

Why can't we use runOnUnitImpl here?

ilya-biryukov marked 2 inline comments as done.

Added more comments to addToEnd/addToFront, added explicit flush instead of relying on raw_ostream's destructor to do it implicitly.

ilya-biryukov marked 2 inline comments as done.May 23 2017, 4:55 AM
ilya-biryukov added inline comments.
clangd/ClangdServer.cpp
85 ↗(On Diff #99773)

Added comments here and at the addToEnd/addToFront methods.

230 ↗(On Diff #99773)

Ok.

clangd/ClangdUnitStore.h
62 ↗(On Diff #99773)

runOnUnitImpl does parsing/reparsing if the ASTFile is not there, which this function doesn't do specifically.

ilya-biryukov added inline comments.May 23 2017, 4:56 AM
clangd/ClangdServer.cpp
102 ↗(On Diff #99773)

"The notifying thread does not need to hold the lock on the same mutex as the one held by the waiting thread(s); in fact doing so is a pessimization, since the notified thread would immediately block again, waiting for the notifying thread to release the lock"
(from: http://en.cppreference.com/w/cpp/thread/condition_variable/notify_one)

krasimir accepted this revision.May 23 2017, 4:59 AM

Looks good.

This revision is now accepted and ready to land.May 23 2017, 4:59 AM
This revision was automatically updated to reflect the committed changes.