[PATCH] D48943: [clangd] Do not write comments into Preamble PCH
This revision was automatically updated to reflect the committed changes.
Closed by commit rCTE336540: [clangd] Do not write comments into Preamble PCH
(authored by ibiryukov, committed by ).
Changed prior to commit:
https://reviews.llvm.org/D48943?vs=154569&id=154572#toc
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48943
Files:
clangd/ClangdUnit.cpp
clangd/CodeCompletionStrings.cpp
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -24,6 +24,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/MacroInfo.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/Sema.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Tooling/CompilationDatabase.h"
@@ -323,6 +324,9 @@
// the preamble and make it smaller.
assert(!CI.getFrontendOpts().SkipFunctionBodies);
CI.getFrontendOpts().SkipFunctionBodies = true;
+ // We don't want to write comment locations into PCH. They are racy and slow
+ // to read back. We rely on dynamic index for the comments instead.
+ CI.getPreprocessorOpts().WriteCommentListToPCH = false;
CppFilePreambleCallbacks SerializedDeclsCollector(FileName,
PreambleCallback);
if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -32,34 +32,6 @@
}
}
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
- bool CommentsFromHeaders) {
- if (CommentsFromHeaders)
-return true;
- auto &SourceMgr = Ctx.getSourceManager();
- // Accessing comments for decls from invalid preamble can lead to crashes.
- // So we only return comments from the main file when doing code completion.
- // For indexing, we still read all the comments.
- // FIXME: find a better fix, e.g. store file contents in the preamble or get
- // doc comments from the index.
- auto canRequestForDecl = [&](const NamedDecl &D) -> bool {
-for (auto *Redecl : D.redecls()) {
- auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
- if (!SourceMgr.isWrittenInMainFile(Loc))
-return false;
-}
-return true;
- };
- // First, check the decl itself.
- if (!canRequestForDecl(D))
-return false;
- // Completion also returns comments for properties, corresponding to ObjC
- // methods.
- const ObjCMethodDecl *M = dyn_cast(&D);
- const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
- return !PDecl || canRequestForDecl(*PDecl);
-}
-
bool LooksLikeDocComment(llvm::StringRef CommentText) {
// We don't report comments that only contain "special" chars.
// This avoids reporting various delimiters, like:
@@ -87,12 +59,13 @@
// the comments for namespaces.
return "";
}
- if (!canRequestComment(Ctx, *Decl, CommentsFromHeaders))
-return "";
-
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
+
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
@@ -104,11 +77,14 @@
const CodeCompleteConsumer::OverloadCandidate &Result,
unsigned ArgIndex, bool CommentsFromHeaders) {
auto *Func = Result.getFunction();
- if (!Func || !canRequestComment(Ctx, *Func, CommentsFromHeaders))
+ if (!Func)
return "";
const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
if (!RC)
return "";
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -24,6 +24,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/MacroInfo.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/Sema.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Tooling/CompilationDatabase.h"
@@ -323,6 +324,9 @@
// the preamble and make it smaller.
assert(!CI.getFrontendOpts().SkipFunctionBodies);
CI.getFrontendOpts().SkipFunctionBodies = true;
+ // We don't want to write comment locations into PCH. They are racy and slow
+ //
[PATCH] D48943: [clangd] Do not write comments into Preamble PCH
ilya-biryukov updated this revision to Diff 154569.
ilya-biryukov marked an inline comment as done.
ilya-biryukov added a comment.
- Fix a comment
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48943
Files:
clangd/ClangdUnit.cpp
clangd/CodeCompletionStrings.cpp
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -32,34 +32,6 @@
}
}
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
- bool CommentsFromHeaders) {
- if (CommentsFromHeaders)
-return true;
- auto &SourceMgr = Ctx.getSourceManager();
- // Accessing comments for decls from invalid preamble can lead to crashes.
- // So we only return comments from the main file when doing code completion.
- // For indexing, we still read all the comments.
- // FIXME: find a better fix, e.g. store file contents in the preamble or get
- // doc comments from the index.
- auto canRequestForDecl = [&](const NamedDecl &D) -> bool {
-for (auto *Redecl : D.redecls()) {
- auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
- if (!SourceMgr.isWrittenInMainFile(Loc))
-return false;
-}
-return true;
- };
- // First, check the decl itself.
- if (!canRequestForDecl(D))
-return false;
- // Completion also returns comments for properties, corresponding to ObjC
- // methods.
- const ObjCMethodDecl *M = dyn_cast(&D);
- const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
- return !PDecl || canRequestForDecl(*PDecl);
-}
-
bool LooksLikeDocComment(llvm::StringRef CommentText) {
// We don't report comments that only contain "special" chars.
// This avoids reporting various delimiters, like:
@@ -87,12 +59,13 @@
// the comments for namespaces.
return "";
}
- if (!canRequestComment(Ctx, *Decl, CommentsFromHeaders))
-return "";
-
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
+
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
@@ -104,11 +77,14 @@
const CodeCompleteConsumer::OverloadCandidate &Result,
unsigned ArgIndex, bool CommentsFromHeaders) {
auto *Func = Result.getFunction();
- if (!Func || !canRequestComment(Ctx, *Func, CommentsFromHeaders))
+ if (!Func)
return "";
const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
if (!RC)
return "";
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -24,6 +24,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/MacroInfo.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/Sema.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Tooling/CompilationDatabase.h"
@@ -323,6 +324,9 @@
// the preamble and make it smaller.
assert(!CI.getFrontendOpts().SkipFunctionBodies);
CI.getFrontendOpts().SkipFunctionBodies = true;
+ // We don't want to write comment locations into PCH. They are racy and slow
+ // to read back. We rely on dynamic index for the comments instead.
+ CI.getPreprocessorOpts().WriteCommentListToPCH = false;
CppFilePreambleCallbacks SerializedDeclsCollector(FileName,
PreambleCallback);
if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -32,34 +32,6 @@
}
}
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
- bool CommentsFromHeaders) {
- if (CommentsFromHeaders)
-return true;
- auto &SourceMgr = Ctx.getSourceManager();
- // Accessing comments for decls from invalid preamble can lead to crashes.
- // So we only return comments from the main file when doing code completion.
- // For indexing, we still read all the comments.
- // FIXME: find a better fix, e.g. store file contents in the preamble or get
- // doc comments from the index.
- auto canRequestForDecl = [&](const NamedDecl &D) -> bool {
-
[PATCH] D48943: [clangd] Do not write comments into Preamble PCH
sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Very nice! Comment at: clangd/ClangdUnit.cpp:327 CI.getFrontendOpts().SkipFunctionBodies = true; + // We don't want to write comments into PCH. They are racy and slow to read + // back. We rely on dynamic index for the comments instead. nit: these are comment *locations* right? (Relevant because if we were writing the actual comments, we may actually want this on!) Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D48943 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D48943: [clangd] Do not write comments into Preamble PCH
ilya-biryukov created this revision.
ilya-biryukov added a reviewer: sammccall.
Herald added subscribers: jkorous, MaskRay, ioeric.
To avoid wasting time deserializing them on code completion and
further reparses.
We do not use the comments anyway, because we cannot rely on the file
contents staying the same for reparses that reuse the prebuilt
preamble PCH.
Repository:
rCTE Clang Tools Extra
https://reviews.llvm.org/D48943
Files:
clangd/ClangdUnit.cpp
clangd/CodeCompletionStrings.cpp
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -32,34 +32,6 @@
}
}
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
- bool CommentsFromHeaders) {
- if (CommentsFromHeaders)
-return true;
- auto &SourceMgr = Ctx.getSourceManager();
- // Accessing comments for decls from invalid preamble can lead to crashes.
- // So we only return comments from the main file when doing code completion.
- // For indexing, we still read all the comments.
- // FIXME: find a better fix, e.g. store file contents in the preamble or get
- // doc comments from the index.
- auto canRequestForDecl = [&](const NamedDecl &D) -> bool {
-for (auto *Redecl : D.redecls()) {
- auto Loc = SourceMgr.getSpellingLoc(Redecl->getLocation());
- if (!SourceMgr.isWrittenInMainFile(Loc))
-return false;
-}
-return true;
- };
- // First, check the decl itself.
- if (!canRequestForDecl(D))
-return false;
- // Completion also returns comments for properties, corresponding to ObjC
- // methods.
- const ObjCMethodDecl *M = dyn_cast(&D);
- const ObjCPropertyDecl *PDecl = M ? M->findPropertyDecl() : nullptr;
- return !PDecl || canRequestForDecl(*PDecl);
-}
-
bool LooksLikeDocComment(llvm::StringRef CommentText) {
// We don't report comments that only contain "special" chars.
// This avoids reporting various delimiters, like:
@@ -87,12 +59,13 @@
// the comments for namespaces.
return "";
}
- if (!canRequestComment(Ctx, *Decl, CommentsFromHeaders))
-return "";
-
const RawComment *RC = getCompletionComment(Ctx, Decl);
if (!RC)
return "";
+
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
@@ -104,11 +77,14 @@
const CodeCompleteConsumer::OverloadCandidate &Result,
unsigned ArgIndex, bool CommentsFromHeaders) {
auto *Func = Result.getFunction();
- if (!Func || !canRequestComment(Ctx, *Func, CommentsFromHeaders))
+ if (!Func)
return "";
const RawComment *RC = getParameterComment(Ctx, Result, ArgIndex);
if (!RC)
return "";
+ // Sanity check that the comment does not come from the PCH. We choose to not
+ // write them into PCH, because they are racy and slow to load.
+ assert(!Ctx.getSourceManager().isLoadedSourceLocation(RC->getLocStart()));
std::string Doc = RC->getFormattedText(Ctx.getSourceManager(),
Ctx.getDiagnostics());
if (!LooksLikeDocComment(Doc))
return "";
Index: clangd/ClangdUnit.cpp
===
--- clangd/ClangdUnit.cpp
+++ clangd/ClangdUnit.cpp
@@ -24,6 +24,7 @@
#include "clang/Lex/Lexer.h"
#include "clang/Lex/MacroInfo.h"
#include "clang/Lex/Preprocessor.h"
+#include "clang/Lex/PreprocessorOptions.h"
#include "clang/Sema/Sema.h"
#include "clang/Serialization/ASTWriter.h"
#include "clang/Tooling/CompilationDatabase.h"
@@ -323,6 +324,9 @@
// the preamble and make it smaller.
assert(!CI.getFrontendOpts().SkipFunctionBodies);
CI.getFrontendOpts().SkipFunctionBodies = true;
+ // We don't want to write comments into PCH. They are racy and slow to read
+ // back. We rely on dynamic index for the comments instead.
+ CI.getPreprocessorOpts().WriteCommentListToPCH = false;
CppFilePreambleCallbacks SerializedDeclsCollector(FileName,
PreambleCallback);
if (Inputs.FS->setCurrentWorkingDirectory(Inputs.CompileCommand.Directory)) {
Index: clangd/CodeCompletionStrings.cpp
===
--- clangd/CodeCompletionStrings.cpp
+++ clangd/CodeCompletionStrings.cpp
@@ -32,34 +32,6 @@
}
}
-bool canRequestComment(const ASTContext &Ctx, const NamedDecl &D,
- bool CommentsFromHeaders) {
- if (CommentsFromHeaders)
-return true;
- auto &SourceMgr = Ctx.getSourceManager();
- // Accessing comments for decls from invalid preamble can lead to crashes.
- // So we only return comments from the main file when doing code completion.
- // For indexing, we sti
