This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Report diagnostics even if WantDiags::No AST was reused
ClosedPublic

Authored by ilya-biryukov on Jul 31 2018, 3:06 AM.

Details

Summary

After r338256, clangd stopped reporting diagnostics if WantDiags::No request
is followed by a WantDiags::Yes request but the AST can be reused.

Event Timeline

ilya-biryukov created this revision.Jul 31 2018, 3:06 AM

Overall LG, just a suggestion to make the code a bit easier to follow.

clangd/TUScheduler.cpp
378

The implicit condition here is that we can reuse AST and diagnostics were not reported. I think we can be more explicit here by moving this early return into the CanReuseAST if branch. By doing this, we could also get rid of PrevDiagsWereReported. An additional state variable seems to have made the flow less clear.

406–410

This was preexisting, but it might worth adding a comment explaining when *AST would be false.

ilya-biryukov marked an inline comment as done.
  • Added a comment
ilya-biryukov added inline comments.Jul 31 2018, 3:35 AM
clangd/TUScheduler.cpp
378

The problem with that would be that the value of DiagsWereReported would be stale for ~40 lines between the assignment to FileInputs and this point.
So I'm not sure about this trade-off. WDYT?

  • Moved early return into the CanReuseAST branch
ilya-biryukov marked 2 inline comments as done.Jul 31 2018, 4:41 AM
ilya-biryukov added inline comments.
clangd/TUScheduler.cpp
378

Update after chatting offline: we agreed to keep the PrevDiagsWereReported and move the early return into the original if branch to make the dependencies between DiagsWereReported and CanReuseAST more explicit. This comment should be done now.

ioeric accepted this revision.Jul 31 2018, 4:42 AM
ioeric added inline comments.
clangd/TUScheduler.cpp
381

I think checking PrevDiagsWereReported here would be more straightforward.

This revision is now accepted and ready to land.Jul 31 2018, 4:42 AM
ilya-biryukov marked 2 inline comments as done.Jul 31 2018, 4:45 AM
ilya-biryukov added inline comments.
clangd/TUScheduler.cpp
381

Good point. Done.

ilya-biryukov marked an inline comment as done.
  • Check for PrevDiagsWereReported
This revision was automatically updated to reflect the committed changes.