This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Simplify code handling compile commands
ClosedPublic

Authored by ilya-biryukov on Jan 17 2018, 6:16 AM.

Details

Event Timeline

ilya-biryukov created this revision.Jan 17 2018, 6:16 AM

This looks better overall to me.

I'm not sure about clangdserver rather than clangdunit computing the commands. You're probably right but I'd like you to explain it to me :-)
Rest is just readability nits.

clangd/ClangdServer.cpp
35

This seems like something of an odd fit: the clangdserver both produces and consumes the compile commands, but ClangdUnit is responsible for storing it?

Are we sure it wouldn't be clearer to keep the "get compile command" action in clangd unit (and propagate the "should rescan" flag), and just encapsulate the resource-dir/fallback logic a bit better?

336

what's inconsistent about this state? (and above)

337

this seems better off inlined?

577

FWIW this might be easier to parse, and avoids dead copies, as

Optional<CompileCommand> ReusedCommand;
if (AllowCachedFlags)
  ReusedCommand = Resources->getLatestCommand();
CompileCommand Cmd = ReusedCommand ? std::move(*ReusedCommand) : getCompileCommand();
ParseInput Inputs(std::move(Cmd), ...)
clangd/ClangdServer.h
335

The name UpdateCompileCommand is confusing in the case that this file hasn't been seen: it's not obvious whether you have to pass true, or whether it doesn't matter.

Consider AllowCachedFlags, and inverting the sense?
At least for me, it's more obvious that this flag is ignored if the cache is empty.
(or AllowCachedCompileCommand, which is a bit long for my taste, or AllowCachedCommand, which is a bit vague)

clangd/ClangdUnit.cpp
479

nit: alias doesn't seem needed

clangd/ClangdUnit.h
62

unwrap?

67

these comments just echo the type/name, remove?

185–186

should ParseInputs be &&? It's big, if callers are copying it something went wrong.

222

nit: consider getLastCommand which removes "latest"'s ambiguity between "previous command used" and "current command, bypassing any caches"

ilya-biryukov marked 7 inline comments as done.

Addressing review comments

ilya-biryukov added inline comments.Jan 18 2018, 9:31 AM
clangd/ClangdServer.cpp
35

I've put the command into ClangdUnit to not change the code consuming compile commands (this is where you currently get compile commands from).

The final goal is to remove compile command from ClangdUnit and store it beside the contents of the file (somewhere inside ClangdServer or its component that manages the resources), ClangdUnit will only manage the built ASTs/Preambles.
This is what ParseInputs is for, it captures all things necessary to build AST/Preamble. When we'll start dropping ASTs for non-accessed files, we could be storing ParseInputs instead to be able to recreate the ASTs when necessary.

336

There's an invariant that if a file is tracked, its compile command in CppFile should be set.
E.g., !!(Untis.getFile(File)) == !!(Untis.getFile(File))->getLatestCompileCommand.

We should probably spell it out explicitly in the assertion message. E.g. "getFile() must only return files with populated commands"
WDYT?

clangd/ClangdServer.h
335

I like CachedFlags, but it also seems a bit vague in the same sense that CachedCommand is vague.
I've opted for AllowCachedCompileFlags, it's long but shouldn't cause any confusion.

clangd/ClangdUnit.h
67

Sorry, I thought I removed them prior to doing the commit.
Thanks for spotting that.

sammccall accepted this revision.Jan 22 2018, 1:41 AM
sammccall added inline comments.
clangd/ClangdUnit.h
62

This is not done. Is clang-format doing this?

(It looks like exactly 80 cols, but in any case the second comma shouldn't be there :-))

64

remove the constructor and just use brace-init?

222

I think that this might actually be the best place to document the invariant you rely on elsewhere (despite it being a layering violation).

// In practice we always call rebuild() when adding a CppFile to the collection,
// and only `cancelRebuild()` after removing it. This means files in the CppFileCollection
// always have a compile command available.
This revision is now accepted and ready to land.Jan 22 2018, 1:41 AM
ilya-biryukov marked 2 inline comments as done.
  • Unwrap the comment
  • Remove ParseInputs constructor
  • Document the CppFile invariant
ilya-biryukov marked an inline comment as done.Jan 23 2018, 7:07 AM
ilya-biryukov added inline comments.
clangd/ClangdUnit.h
62

Sorry, my mistake. Fixed now.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.