Page MenuHomePhabricator

[clangd] Check if CompileCommand has changed on forceReparse.
ClosedPublic

Authored by ilya-biryukov on Aug 7 2017, 7:53 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

ilya-biryukov created this revision.Aug 7 2017, 7:53 AM
klimek added inline comments.Aug 7 2017, 8:01 AM
clangd/ClangdUnitStore.cpp
45 ↗(On Diff #109994)

Just say RemovedFile = nullptr in the struct?

clangd/ClangdUnitStore.h
45–48 ↗(On Diff #109994)

Not obvious to me what things in there mean.

  • Moved assignment RemoveFile = nullptr around a bit.
  • Added a comment to recreateFileIfCompileCommandChanged.
clangd/ClangdUnitStore.cpp
45 ↗(On Diff #109994)

I'd argue that having it in the code, rather than declaration makes it more readable. Besides, default ctor of shared_ptr gives us nullptr anyway, so we can also leave it as is and remove the assignments altogether.

I've moved =nullptr assignments out of the if/else bodies. Let me know if it still looks ugly.

clangd/ClangdUnitStore.h
45–48 ↗(On Diff #109994)

Added a comment. Hopefully makes sense now.

klimek edited edge metadata.Aug 8 2017, 1:17 AM

Also missing tests :)

clangd/ClangdUnitStore.cpp
45 ↗(On Diff #109994)

Ah, I missed that it's a smart pointer; in that case, yes, remove the = nullptr.

clangd/ClangdUnitStore.h
45–48 ↗(On Diff #109994)

Better, thanks. Now, why does this need to be shared_ptr (as opposed to unique_ptr)? Don't we always only have one?

Addressed review comments.

  • Added a test for forceReparse.
  • Got rid of a redundant = nullptr assignment.
ilya-biryukov marked an inline comment as done.Aug 8 2017, 2:14 AM

Also missing tests :)

Done :-)

clangd/ClangdUnitStore.h
45–48 ↗(On Diff #109994)

This CppFile may still be referenced from the worker threads that store a pending reparse task for this file. Or there may even be an active reparse on a separate thread.

klimek accepted this revision.Aug 11 2017, 1:51 AM

lg

unittests/clangd/ClangdTests.cpp
509–510 ↗(On Diff #110155)

Nit: Comment fits in one line.

This revision is now accepted and ready to land.Aug 11 2017, 1:51 AM
This revision was automatically updated to reflect the committed changes.