[PATCH] D42173: [clangd] Simplify code handling compile commands
This revision was automatically updated to reflect the committed changes.
Closed by commit rL323204: [clangd] Simplify code handling compile commands
(authored by ibiryukov, committed by ).
Herald added a subscriber: llvm-commits.
Repository:
rL LLVM
https://reviews.llvm.org/D42173
Files:
clang-tools-extra/trunk/clangd/ClangdServer.cpp
clang-tools-extra/trunk/clangd/ClangdServer.h
clang-tools-extra/trunk/clangd/ClangdUnit.cpp
clang-tools-extra/trunk/clangd/ClangdUnit.h
clang-tools-extra/trunk/clangd/ClangdUnitStore.cpp
clang-tools-extra/trunk/clangd/ClangdUnitStore.h
Index: clang-tools-extra/trunk/clangd/ClangdUnit.cpp
===
--- clang-tools-extra/trunk/clangd/ClangdUnit.cpp
+++ clang-tools-extra/trunk/clangd/ClangdUnit.cpp
@@ -273,7 +273,6 @@
return Mgr.getMacroArgExpandedLocation(InputLoc);
}
-
} // namespace
void ParsedAST::ensurePreambleDeclsDeserialized() {
@@ -359,27 +358,22 @@
: AST(std::move(AST)) {}
std::shared_ptr
-CppFile::Create(PathRef FileName, tooling::CompileCommand Command,
-bool StorePreamblesInMemory,
+CppFile::Create(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr PCHs,
ASTParsedCallback ASTCallback) {
- return std::shared_ptr(
- new CppFile(FileName, std::move(Command), StorePreamblesInMemory,
- std::move(PCHs), std::move(ASTCallback)));
+ return std::shared_ptr(new CppFile(FileName, StorePreamblesInMemory,
+ std::move(PCHs),
+ std::move(ASTCallback)));
}
-CppFile::CppFile(PathRef FileName, tooling::CompileCommand Command,
- bool StorePreamblesInMemory,
+CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr PCHs,
ASTParsedCallback ASTCallback)
-: FileName(FileName), Command(std::move(Command)),
- StorePreamblesInMemory(StorePreamblesInMemory), RebuildCounter(0),
- RebuildInProgress(false), PCHs(std::move(PCHs)),
+: FileName(FileName), StorePreamblesInMemory(StorePreamblesInMemory),
+ RebuildCounter(0), RebuildInProgress(false), PCHs(std::move(PCHs)),
ASTCallback(std::move(ASTCallback)) {
// FIXME(ibiryukov): we should pass a proper Context here.
- log(Context::empty(), "Opened file " + FileName + " with command [" +
-this->Command.Directory + "] " +
-llvm::join(this->Command.CommandLine, " "));
+ log(Context::empty(), "Created CppFile for " + FileName);
std::lock_guard Lock(Mutex);
LatestAvailablePreamble = nullptr;
@@ -431,14 +425,12 @@
}
llvm::Optional
-CppFile::rebuild(const Context , StringRef NewContents,
- IntrusiveRefCntPtr VFS) {
- return deferRebuild(NewContents, std::move(VFS))(Ctx);
+CppFile::rebuild(const Context , ParseInputs &) {
+ return deferRebuild(std::move(Inputs))(Ctx);
}
UniqueFunction(const Context &)>
-CppFile::deferRebuild(StringRef NewContents,
- IntrusiveRefCntPtr VFS) {
+CppFile::deferRebuild(ParseInputs &) {
std::shared_ptr OldPreamble;
std::shared_ptr PCHs;
unsigned RequestRebuildCounter;
@@ -463,6 +455,7 @@
this->ASTPromise = std::promise();
this->ASTFuture = this->ASTPromise.get_future();
}
+this->LastCommand = Inputs.CompileCommand;
} // unlock Mutex.
// Notify about changes to RebuildCounter.
RebuildCond.notify_all();
@@ -473,22 +466,27 @@
// Don't let this CppFile die before rebuild is finished.
std::shared_ptr That = shared_from_this();
auto FinishRebuild =
- [OldPreamble, VFS, RequestRebuildCounter, PCHs,
- That](std::string NewContents,
+ [OldPreamble, RequestRebuildCounter, PCHs,
+ That](ParseInputs Inputs,
const Context ) mutable /* to allow changing OldPreamble. */
-> llvm::Optional {
+log(Context::empty(),
+"Rebuilding file " + That->FileName + " with command [" +
+Inputs.CompileCommand.Directory + "] " +
+llvm::join(Inputs.CompileCommand.CommandLine, " "));
+
// Only one execution of this method is possible at a time.
// RebuildGuard will wait for any ongoing rebuilds to finish and will put us
// into a state for doing a rebuild.
RebuildGuard Rebuild(*That, RequestRebuildCounter);
if (Rebuild.wasCancelledBeforeConstruction())
return llvm::None;
std::vector ArgStrs;
-for (const auto : That->Command.CommandLine)
+for (const auto : Inputs.CompileCommand.CommandLine)
ArgStrs.push_back(S.c_str());
-VFS->setCurrentWorkingDirectory(That->Command.Directory);
+Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory);
std::unique_ptr CI;
{
@@ -498,25 +496,26 @@
[PATCH] D42173: [clangd] Simplify code handling compile commands
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE323204: [clangd] Simplify code handling compile commands
(authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D42173?vs=131062=131067#toc
Repository:
rL LLVM
https://reviews.llvm.org/D42173
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -273,7 +273,6 @@
return Mgr.getMacroArgExpandedLocation(InputLoc);
}
-
} // namespace
void ParsedAST::ensurePreambleDeclsDeserialized() {
@@ -359,27 +358,22 @@
: AST(std::move(AST)) {}
std::shared_ptr
-CppFile::Create(PathRef FileName, tooling::CompileCommand Command,
-bool StorePreamblesInMemory,
+CppFile::Create(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr PCHs,
ASTParsedCallback ASTCallback) {
- return std::shared_ptr(
- new CppFile(FileName, std::move(Command), StorePreamblesInMemory,
- std::move(PCHs), std::move(ASTCallback)));
+ return std::shared_ptr(new CppFile(FileName, StorePreamblesInMemory,
+ std::move(PCHs),
+ std::move(ASTCallback)));
}
-CppFile::CppFile(PathRef FileName, tooling::CompileCommand Command,
- bool StorePreamblesInMemory,
+CppFile::CppFile(PathRef FileName, bool StorePreamblesInMemory,
std::shared_ptr PCHs,
ASTParsedCallback ASTCallback)
-: FileName(FileName), Command(std::move(Command)),
- StorePreamblesInMemory(StorePreamblesInMemory), RebuildCounter(0),
- RebuildInProgress(false), PCHs(std::move(PCHs)),
+: FileName(FileName), StorePreamblesInMemory(StorePreamblesInMemory),
+ RebuildCounter(0), RebuildInProgress(false), PCHs(std::move(PCHs)),
ASTCallback(std::move(ASTCallback)) {
// FIXME(ibiryukov): we should pass a proper Context here.
- log(Context::empty(), "Opened file " + FileName + " with command [" +
-this->Command.Directory + "] " +
-llvm::join(this->Command.CommandLine, " "));
+ log(Context::empty(), "Created CppFile for " + FileName);
std::lock_guard Lock(Mutex);
LatestAvailablePreamble = nullptr;
@@ -431,14 +425,12 @@
}
llvm::Optional
-CppFile::rebuild(const Context , StringRef NewContents,
- IntrusiveRefCntPtr VFS) {
- return deferRebuild(NewContents, std::move(VFS))(Ctx);
+CppFile::rebuild(const Context , ParseInputs &) {
+ return deferRebuild(std::move(Inputs))(Ctx);
}
UniqueFunction(const Context &)>
-CppFile::deferRebuild(StringRef NewContents,
- IntrusiveRefCntPtr VFS) {
+CppFile::deferRebuild(ParseInputs &) {
std::shared_ptr OldPreamble;
std::shared_ptr PCHs;
unsigned RequestRebuildCounter;
@@ -463,6 +455,7 @@
this->ASTPromise = std::promise();
this->ASTFuture = this->ASTPromise.get_future();
}
+this->LastCommand = Inputs.CompileCommand;
} // unlock Mutex.
// Notify about changes to RebuildCounter.
RebuildCond.notify_all();
@@ -473,22 +466,27 @@
// Don't let this CppFile die before rebuild is finished.
std::shared_ptr That = shared_from_this();
auto FinishRebuild =
- [OldPreamble, VFS, RequestRebuildCounter, PCHs,
- That](std::string NewContents,
+ [OldPreamble, RequestRebuildCounter, PCHs,
+ That](ParseInputs Inputs,
const Context ) mutable /* to allow changing OldPreamble. */
-> llvm::Optional {
+log(Context::empty(),
+"Rebuilding file " + That->FileName + " with command [" +
+Inputs.CompileCommand.Directory + "] " +
+llvm::join(Inputs.CompileCommand.CommandLine, " "));
+
// Only one execution of this method is possible at a time.
// RebuildGuard will wait for any ongoing rebuilds to finish and will put us
// into a state for doing a rebuild.
RebuildGuard Rebuild(*That, RequestRebuildCounter);
if (Rebuild.wasCancelledBeforeConstruction())
return llvm::None;
std::vector ArgStrs;
-for (const auto : That->Command.CommandLine)
+for (const auto : Inputs.CompileCommand.CommandLine)
ArgStrs.push_back(S.c_str());
-VFS->setCurrentWorkingDirectory(That->Command.Directory);
+Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory);
std::unique_ptr CI;
{
@@ -498,25 +496,26 @@
IntrusiveRefCntPtr CommandLineDiagsEngine =
CompilerInstance::createDiagnostics(new DiagnosticOptions,
, false);
-
[PATCH] D42173: [clangd] Simplify code handling compile commands
ilya-biryukov marked an inline comment as done.
ilya-biryukov added inline comments.
Comment at: clangd/ClangdUnit.h:62
+/// Information required to run clang (e.g., to parse AST or do code
+/// completion).
+struct ParseInputs {
sammccall wrote:
> sammccall wrote:
> > unwrap?
> 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 :-))
Sorry, my mistake. Fixed now.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42173: [clangd] Simplify code handling compile commands
ilya-biryukov updated this revision to Diff 131062.
ilya-biryukov marked 2 inline comments as done.
ilya-biryukov added a comment.
- Unwrap the comment
- Remove ParseInputs constructor
- Document the CppFile invariant
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnitStore.h
===
--- clangd/ClangdUnitStore.h
+++ clangd/ClangdUnitStore.h
@@ -32,42 +32,19 @@
std::shared_ptr
getOrCreateFile(PathRef File, PathRef ResourceDir,
- GlobalCompilationDatabase , bool StorePreamblesInMemory,
+ bool StorePreamblesInMemory,
std::shared_ptr PCHs) {
std::lock_guard Lock(Mutex);
-
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end()) {
- auto Command = getCompileCommand(CDB, File, ResourceDir);
-
It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(Command),
- StorePreamblesInMemory,
+ .try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
std::move(PCHs), ASTCallback))
.first;
}
return It->second;
}
- struct RecreateResult {
-/// A CppFile, stored in this CppFileCollection for the corresponding
-/// filepath after calling recreateFileIfCompileCommandChanged.
-std::shared_ptr FileInCollection;
-/// If a new CppFile had to be created to account for changed
-/// CompileCommand, a previous CppFile instance will be returned in this
-/// field.
-std::shared_ptr RemovedFile;
- };
-
- /// Similar to getOrCreateFile, but will replace a current CppFile for \p File
- /// with a new one if CompileCommand, provided by \p CDB has changed.
- /// If a currently stored CppFile had to be replaced, the previous instance
- /// will be returned in RecreateResult.RemovedFile.
- RecreateResult recreateFileIfCompileCommandChanged(
- PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
- bool StorePreamblesInMemory,
- std::shared_ptr PCHs);
-
std::shared_ptr getFile(PathRef File) {
std::lock_guard Lock(Mutex);
@@ -82,12 +59,6 @@
std::shared_ptr removeIfPresent(PathRef File);
private:
- tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase ,
-PathRef File, PathRef ResourceDir);
-
- bool compileCommandsAreEqual(tooling::CompileCommand const ,
- tooling::CompileCommand const );
-
std::mutex Mutex;
llvm::StringMap OpenedFiles;
ASTParsedCallback ASTCallback;
Index: clangd/ClangdUnitStore.cpp
===
--- clangd/ClangdUnitStore.cpp
+++ clangd/ClangdUnitStore.cpp
@@ -25,53 +25,3 @@
OpenedFiles.erase(It);
return Result;
}
-
-CppFileCollection::RecreateResult
-CppFileCollection::recreateFileIfCompileCommandChanged(
-PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
-bool StorePreamblesInMemory, std::shared_ptr PCHs) {
- auto NewCommand = getCompileCommand(CDB, File, ResourceDir);
-
- std::lock_guard Lock(Mutex);
-
- RecreateResult Result;
-
- auto It = OpenedFiles.find(File);
- if (It == OpenedFiles.end()) {
-It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(NewCommand),
-StorePreamblesInMemory,
-std::move(PCHs), ASTCallback))
- .first;
- } else if (!compileCommandsAreEqual(It->second->getCompileCommand(),
- NewCommand)) {
-Result.RemovedFile = std::move(It->second);
-It->second =
-CppFile::Create(File, std::move(NewCommand), StorePreamblesInMemory,
-std::move(PCHs), ASTCallback);
- }
- Result.FileInCollection = It->second;
- return Result;
-}
-
-tooling::CompileCommand
-CppFileCollection::getCompileCommand(GlobalCompilationDatabase ,
- PathRef File, PathRef ResourceDir) {
- llvm::Optional C = CDB.getCompileCommand(File);
- if (!C) // FIXME: Suppress diagnostics? Let the user know?
-C = CDB.getFallbackCommand(File);
-
- // Inject the resource dir.
- // FIXME: Don't overwrite it if it's already there.
- C->CommandLine.push_back("-resource-dir=" + ResourceDir.str());
- return std::move(*C);
-}
-
-bool CppFileCollection::compileCommandsAreEqual(
-tooling::CompileCommand const , tooling::CompileCommand const ) {
- // tooling::CompileCommand.Output is ignored, it's not relevant for clangd.
- return LHS.Directory == RHS.Directory &&
- LHS.CommandLine.size()
[PATCH] D42173: [clangd] Simplify code handling compile commands
sammccall accepted this revision.
sammccall added inline comments.
This revision is now accepted and ready to land.
Comment at: clangd/ClangdUnit.h:62
+/// Information required to run clang (e.g., to parse AST or do code
+/// completion).
+struct ParseInputs {
sammccall wrote:
> unwrap?
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 :-))
Comment at: clangd/ClangdUnit.h:64
+struct ParseInputs {
+ ParseInputs(tooling::CompileCommand CompileCommand,
+ IntrusiveRefCntPtr FS, std::string Contents);
remove the constructor and just use brace-init?
Comment at: clangd/ClangdUnit.h:226
+ /// cancelRebuild().
+ llvm::Optional getLastCommand() const;
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.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42173: [clangd] Simplify code handling compile commands
ilya-biryukov updated this revision to Diff 130428.
ilya-biryukov marked 7 inline comments as done.
ilya-biryukov added a comment.
Herald added a subscriber: ioeric.
Addressing review comments
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnitStore.h
===
--- clangd/ClangdUnitStore.h
+++ clangd/ClangdUnitStore.h
@@ -32,42 +32,19 @@
std::shared_ptr
getOrCreateFile(PathRef File, PathRef ResourceDir,
- GlobalCompilationDatabase , bool StorePreamblesInMemory,
+ bool StorePreamblesInMemory,
std::shared_ptr PCHs) {
std::lock_guard Lock(Mutex);
-
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end()) {
- auto Command = getCompileCommand(CDB, File, ResourceDir);
-
It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(Command),
- StorePreamblesInMemory,
+ .try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
std::move(PCHs), ASTCallback))
.first;
}
return It->second;
}
- struct RecreateResult {
-/// A CppFile, stored in this CppFileCollection for the corresponding
-/// filepath after calling recreateFileIfCompileCommandChanged.
-std::shared_ptr FileInCollection;
-/// If a new CppFile had to be created to account for changed
-/// CompileCommand, a previous CppFile instance will be returned in this
-/// field.
-std::shared_ptr RemovedFile;
- };
-
- /// Similar to getOrCreateFile, but will replace a current CppFile for \p File
- /// with a new one if CompileCommand, provided by \p CDB has changed.
- /// If a currently stored CppFile had to be replaced, the previous instance
- /// will be returned in RecreateResult.RemovedFile.
- RecreateResult recreateFileIfCompileCommandChanged(
- PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
- bool StorePreamblesInMemory,
- std::shared_ptr PCHs);
-
std::shared_ptr getFile(PathRef File) {
std::lock_guard Lock(Mutex);
@@ -82,12 +59,6 @@
std::shared_ptr removeIfPresent(PathRef File);
private:
- tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase ,
-PathRef File, PathRef ResourceDir);
-
- bool compileCommandsAreEqual(tooling::CompileCommand const ,
- tooling::CompileCommand const );
-
std::mutex Mutex;
llvm::StringMap OpenedFiles;
ASTParsedCallback ASTCallback;
Index: clangd/ClangdUnitStore.cpp
===
--- clangd/ClangdUnitStore.cpp
+++ clangd/ClangdUnitStore.cpp
@@ -25,53 +25,3 @@
OpenedFiles.erase(It);
return Result;
}
-
-CppFileCollection::RecreateResult
-CppFileCollection::recreateFileIfCompileCommandChanged(
-PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
-bool StorePreamblesInMemory, std::shared_ptr PCHs) {
- auto NewCommand = getCompileCommand(CDB, File, ResourceDir);
-
- std::lock_guard Lock(Mutex);
-
- RecreateResult Result;
-
- auto It = OpenedFiles.find(File);
- if (It == OpenedFiles.end()) {
-It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(NewCommand),
-StorePreamblesInMemory,
-std::move(PCHs), ASTCallback))
- .first;
- } else if (!compileCommandsAreEqual(It->second->getCompileCommand(),
- NewCommand)) {
-Result.RemovedFile = std::move(It->second);
-It->second =
-CppFile::Create(File, std::move(NewCommand), StorePreamblesInMemory,
-std::move(PCHs), ASTCallback);
- }
- Result.FileInCollection = It->second;
- return Result;
-}
-
-tooling::CompileCommand
-CppFileCollection::getCompileCommand(GlobalCompilationDatabase ,
- PathRef File, PathRef ResourceDir) {
- llvm::Optional C = CDB.getCompileCommand(File);
- if (!C) // FIXME: Suppress diagnostics? Let the user know?
-C = CDB.getFallbackCommand(File);
-
- // Inject the resource dir.
- // FIXME: Don't overwrite it if it's already there.
- C->CommandLine.push_back("-resource-dir=" + ResourceDir.str());
- return std::move(*C);
-}
-
-bool CppFileCollection::compileCommandsAreEqual(
-tooling::CompileCommand const , tooling::CompileCommand const ) {
- // tooling::CompileCommand.Output is ignored, it's not relevant for clangd.
- return LHS.Directory == RHS.Directory &&
- LHS.CommandLine.size() == RHS.CommandLine.size()
[PATCH] D42173: [clangd] Simplify code handling compile commands
ilya-biryukov added inline comments.
Comment at: clangd/ClangdServer.cpp:35
+tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase ,
+ PathRef File, PathRef ResourceDir) {
sammccall wrote:
> 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?
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.
Comment at: clangd/ClangdServer.cpp:336
+ assert(Resources->getLatestCommand() &&
+ "CppFile is in inconsistent state, missing CompileCommand");
+ tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
sammccall wrote:
> what's inconsistent about this state? (and above)
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?
Comment at: clangd/ClangdServer.h:335
+ Tagged TaggedFS,
+ bool UpdateCompileCommand);
sammccall wrote:
> 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)
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.
Comment at: clangd/ClangdUnit.h:67
+
+ /// Compilation arguments.
+ tooling::CompileCommand CompileCommand;
sammccall wrote:
> these comments just echo the type/name, remove?
Sorry, I thought I removed them prior to doing the commit.
Thanks for spotting that.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42173: [clangd] Simplify code handling compile commands
sammccall added a comment.
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.
Comment at: clangd/ClangdServer.cpp:35
+tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase ,
+ PathRef File, PathRef ResourceDir) {
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?
Comment at: clangd/ClangdServer.cpp:336
+ assert(Resources->getLatestCommand() &&
+ "CppFile is in inconsistent state, missing CompileCommand");
+ tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
what's inconsistent about this state? (and above)
Comment at: clangd/ClangdServer.cpp:337
+ "CppFile is in inconsistent state, missing CompileCommand");
+ tooling::CompileCommand CompileCommand = *Resources->getLatestCommand();
+
this seems better off inlined?
Comment at: clangd/ClangdServer.cpp:577
+
+ llvm::Optional FileCmd =
+ Resources->getLatestCommand();
FWIW this might be easier to parse, and avoids dead copies, as
Optional ReusedCommand;
if (AllowCachedFlags)
ReusedCommand = Resources->getLatestCommand();
CompileCommand Cmd = ReusedCommand ? std::move(*ReusedCommand) :
getCompileCommand();
ParseInput Inputs(std::move(Cmd), ...)
Comment at: clangd/ClangdServer.h:335
+ Tagged TaggedFS,
+ bool UpdateCompileCommand);
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)
Comment at: clangd/ClangdUnit.cpp:485
+auto = Inputs.FS;
+auto = Inputs.Contents;
+
nit: alias doesn't seem needed
Comment at: clangd/ClangdUnit.h:62
+/// Information required to run clang (e.g., to parse AST or do code
+/// completion).
+struct ParseInputs {
unwrap?
Comment at: clangd/ClangdUnit.h:67
+
+ /// Compilation arguments.
+ tooling::CompileCommand CompileCommand;
these comments just echo the type/name, remove?
Comment at: clangd/ClangdUnit.h:192
/// diagnostics were produced.
- llvm::Optional
- rebuild(const Context , StringRef NewContents,
- IntrusiveRefCntPtr VFS);
+ llvm::Optional rebuild(const Context ,
+ ParseInputs Inputs);
should ParseInputs be &&? It's big, if callers are copying it something went
wrong.
Comment at: clangd/ClangdUnit.h:229
+ /// cancelRebuild().
+ llvm::Optional getLatestCommand() const;
nit: consider `getLastCommand` which removes "latest"'s ambiguity between
"previous command used" and "current command, bypassing any caches"
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D42173: [clangd] Simplify code handling compile commands
ilya-biryukov created this revision.
ilya-biryukov added reviewers: sammccall, bkramer.
Herald added a subscriber: klimek.
CppFile can now change compilation arguments during rebuild. This allows
simplifying code that manages CppFiles.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D42173
Files:
clangd/ClangdServer.cpp
clangd/ClangdServer.h
clangd/ClangdUnit.cpp
clangd/ClangdUnit.h
clangd/ClangdUnitStore.cpp
clangd/ClangdUnitStore.h
Index: clangd/ClangdUnitStore.h
===
--- clangd/ClangdUnitStore.h
+++ clangd/ClangdUnitStore.h
@@ -32,42 +32,19 @@
std::shared_ptr
getOrCreateFile(PathRef File, PathRef ResourceDir,
- GlobalCompilationDatabase , bool StorePreamblesInMemory,
+ bool StorePreamblesInMemory,
std::shared_ptr PCHs) {
std::lock_guard Lock(Mutex);
-
auto It = OpenedFiles.find(File);
if (It == OpenedFiles.end()) {
- auto Command = getCompileCommand(CDB, File, ResourceDir);
-
It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(Command),
- StorePreamblesInMemory,
+ .try_emplace(File, CppFile::Create(File, StorePreamblesInMemory,
std::move(PCHs), ASTCallback))
.first;
}
return It->second;
}
- struct RecreateResult {
-/// A CppFile, stored in this CppFileCollection for the corresponding
-/// filepath after calling recreateFileIfCompileCommandChanged.
-std::shared_ptr FileInCollection;
-/// If a new CppFile had to be created to account for changed
-/// CompileCommand, a previous CppFile instance will be returned in this
-/// field.
-std::shared_ptr RemovedFile;
- };
-
- /// Similar to getOrCreateFile, but will replace a current CppFile for \p File
- /// with a new one if CompileCommand, provided by \p CDB has changed.
- /// If a currently stored CppFile had to be replaced, the previous instance
- /// will be returned in RecreateResult.RemovedFile.
- RecreateResult recreateFileIfCompileCommandChanged(
- PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
- bool StorePreamblesInMemory,
- std::shared_ptr PCHs);
-
std::shared_ptr getFile(PathRef File) {
std::lock_guard Lock(Mutex);
@@ -82,12 +59,6 @@
std::shared_ptr removeIfPresent(PathRef File);
private:
- tooling::CompileCommand getCompileCommand(GlobalCompilationDatabase ,
-PathRef File, PathRef ResourceDir);
-
- bool compileCommandsAreEqual(tooling::CompileCommand const ,
- tooling::CompileCommand const );
-
std::mutex Mutex;
llvm::StringMap OpenedFiles;
ASTParsedCallback ASTCallback;
Index: clangd/ClangdUnitStore.cpp
===
--- clangd/ClangdUnitStore.cpp
+++ clangd/ClangdUnitStore.cpp
@@ -25,53 +25,3 @@
OpenedFiles.erase(It);
return Result;
}
-
-CppFileCollection::RecreateResult
-CppFileCollection::recreateFileIfCompileCommandChanged(
-PathRef File, PathRef ResourceDir, GlobalCompilationDatabase ,
-bool StorePreamblesInMemory, std::shared_ptr PCHs) {
- auto NewCommand = getCompileCommand(CDB, File, ResourceDir);
-
- std::lock_guard Lock(Mutex);
-
- RecreateResult Result;
-
- auto It = OpenedFiles.find(File);
- if (It == OpenedFiles.end()) {
-It = OpenedFiles
- .try_emplace(File, CppFile::Create(File, std::move(NewCommand),
-StorePreamblesInMemory,
-std::move(PCHs), ASTCallback))
- .first;
- } else if (!compileCommandsAreEqual(It->second->getCompileCommand(),
- NewCommand)) {
-Result.RemovedFile = std::move(It->second);
-It->second =
-CppFile::Create(File, std::move(NewCommand), StorePreamblesInMemory,
-std::move(PCHs), ASTCallback);
- }
- Result.FileInCollection = It->second;
- return Result;
-}
-
-tooling::CompileCommand
-CppFileCollection::getCompileCommand(GlobalCompilationDatabase ,
- PathRef File, PathRef ResourceDir) {
- llvm::Optional C = CDB.getCompileCommand(File);
- if (!C) // FIXME: Suppress diagnostics? Let the user know?
-C = CDB.getFallbackCommand(File);
-
- // Inject the resource dir.
- // FIXME: Don't overwrite it if it's already there.
- C->CommandLine.push_back("-resource-dir=" + ResourceDir.str());
- return std::move(*C);
-}
-
-bool CppFileCollection::compileCommandsAreEqual(
-tooling::CompileCommand const , tooling::CompileCommand const ) {
- // tooling::CompileCommand.Output is ignored, it's not relevant for clangd.
- return LHS.Directory == RHS.Directory &&
-
