CppFile can now change compilation arguments during rebuild. This allows
simplifying code that manages CppFiles.
Details
Diff Detail
- Repository
- rCTE Clang Tools Extra
Event Timeline
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? | |
585 | 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? | |
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" |
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. | |
336 | There's an invariant that if a file is tracked, its compile command in CppFile should be set. We should probably spell it out explicitly in the assertion message. E.g. "getFile() must only return files with populated commands" | |
clangd/ClangdServer.h | ||
335 | I like CachedFlags, but it also seems a bit vague in the same sense that CachedCommand is vague. | |
clangd/ClangdUnit.h | ||
67 | Sorry, I thought I removed them prior to doing the commit. |
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. |
clangd/ClangdUnit.h | ||
---|---|---|
62 | Sorry, my mistake. Fixed now. |
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)