upsj updated this revision to Diff 429653.
upsj marked 4 inline comments as done.
upsj added a comment.
review updates
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124688/new/
https://reviews.llvm.org/D124688
Files:
clang-tools-extra/clangd/ClangdServer.cpp
clang-tools-extra/clangd/ClangdServer.h
clang-tools-extra/clangd/Compiler.h
clang-tools-extra/clangd/Preamble.cpp
clang-tools-extra/clangd/tool/Check.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
clang-tools-extra/clangd/unittests/TestTU.cpp
clang-tools-extra/clangd/unittests/TestTU.h
Index: clang-tools-extra/clangd/unittests/TestTU.h
===================================================================
--- clang-tools-extra/clangd/unittests/TestTU.h
+++ clang-tools-extra/clangd/unittests/TestTU.h
@@ -66,6 +66,9 @@
// Simulate a header guard of the header (using an #import directive).
bool ImplicitHeaderGuard = true;
+ // Parse options pass on to the ParseInputs
+ ParseOptions ParseOpts = {};
+
// Whether to use overlay the TestFS over the real filesystem. This is
// required for use of implicit modules.where the module file is written to
// disk and later read back.
Index: clang-tools-extra/clangd/unittests/TestTU.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/TestTU.cpp
+++ clang-tools-extra/clangd/unittests/TestTU.cpp
@@ -107,6 +107,7 @@
ParsedAST TestTU::build() const {
MockFS FS;
auto Inputs = inputs(FS);
+ Inputs.Opts = ParseOpts;
StoreDiags Diags;
auto CI = buildCompilerInvocation(Inputs, Diags);
assert(CI && "Failed to build compilation invocation.");
Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -389,7 +389,7 @@
namespace std {
// These mocks aren't quite right - we omit unique_ptr for simplicity.
// forward is included to show its body is not needed to get the diagnostic.
- template <typename T> T&& forward(T& t) { return static_cast<T&&>(t); }
+ template <typename T> T&& forward(T& t);
template <typename T, typename... A> T* make_unique(A&&... args) {
return new T(std::forward<A>(args)...);
}
@@ -402,6 +402,32 @@
"no matching constructor for initialization of 'S'")));
}
+TEST(DiagnosticTest, MakeShared) {
+ // We usually miss diagnostics from header functions as we don't parse them.
+ // std::make_shared is only parsed when --parse-forwarding-functions is set
+ Annotations Main(R"cpp(
+ struct S { S(char*); };
+ auto x = std::[[make_shared]]<S>(42); // error-ok
+ )cpp");
+ TestTU TU = TestTU::withCode(Main.code());
+ TU.HeaderCode = R"cpp(
+ namespace std {
+ // These mocks aren't quite right - we omit shared_ptr for simplicity.
+ // forward is included to show its body is not needed to get the diagnostic.
+ template <typename T> T&& forward(T& t);
+ template <typename T, typename... A> T* make_shared(A&&... args) {
+ return new T(std::forward<A>(args)...);
+ }
+ }
+ )cpp";
+ TU.ParseOpts.PreambleParseForwardingFunctions = true;
+ EXPECT_THAT(*TU.build().getDiagnostics(),
+ UnorderedElementsAre(
+ Diag(Main.range(),
+ "in template: "
+ "no matching constructor for initialization of 'S'")));
+}
+
TEST(DiagnosticTest, NoMultipleDiagnosticInFlight) {
Annotations Main(R"cpp(
template <typename T> struct Foo {
Index: clang-tools-extra/clangd/tool/ClangdMain.cpp
===================================================================
--- clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ clang-tools-extra/clangd/tool/ClangdMain.cpp
@@ -498,6 +498,14 @@
Hidden,
init(ClangdServer::Options().UseDirtyHeaders)};
+opt<bool> PreambleParseForwardingFunctions{
+ "parse-forwarding-functions",
+ cat(Misc),
+ desc("Parse all emplace-like functions in included headers"),
+ Hidden,
+ init(ParseOptions().PreambleParseForwardingFunctions),
+};
+
#if defined(__GLIBC__) && CLANGD_MALLOC_TRIM
opt<bool> EnableMallocTrim{
"malloc-trim",
@@ -935,6 +943,7 @@
Opts.ClangTidyProvider = ClangTidyOptProvider;
}
Opts.UseDirtyHeaders = UseDirtyHeaders;
+ Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
Opts.QueryDriverGlobs = std::move(QueryDriverGlobs);
Opts.TweakFilter = [&](const Tweak &T) {
if (T.hidden() && !HiddenFeatures)
Index: clang-tools-extra/clangd/tool/Check.cpp
===================================================================
--- clang-tools-extra/clangd/tool/Check.cpp
+++ clang-tools-extra/clangd/tool/Check.cpp
@@ -126,6 +126,8 @@
Inputs.CompileCommand = Cmd;
Inputs.TFS = &TFS;
Inputs.ClangTidyProvider = Opts.ClangTidyProvider;
+ Inputs.Opts.PreambleParseForwardingFunctions =
+ Opts.PreambleParseForwardingFunctions;
if (Contents.hasValue()) {
Inputs.Contents = *Contents;
log("Imaginary source file contents:\n{0}", Inputs.Contents);
Index: clang-tools-extra/clangd/Preamble.cpp
===================================================================
--- clang-tools-extra/clangd/Preamble.cpp
+++ clang-tools-extra/clangd/Preamble.cpp
@@ -66,9 +66,10 @@
public:
CppFilePreambleCallbacks(
PathRef File, PreambleParsedCallback ParsedCallback,
- PreambleBuildStats *Stats,
+ PreambleBuildStats *Stats, bool ParseForwardingFunctions,
std::function<void(CompilerInstance &)> BeforeExecuteCallback)
: File(File), ParsedCallback(ParsedCallback), Stats(Stats),
+ ParseForwardingFunctions(ParseForwardingFunctions),
BeforeExecuteCallback(std::move(BeforeExecuteCallback)) {}
IncludeStructure takeIncludes() { return std::move(Includes); }
@@ -136,15 +137,48 @@
return IWYUHandler.get();
}
+ static bool isLikelyForwardingFunction(FunctionTemplateDecl *FT) {
+ const auto *FD = FT->getTemplatedDecl();
+ const auto NumParams = FD->getNumParams();
+ // Check whether its last parameter is a parameter pack...
+ if (NumParams > 0) {
+ const auto *LastParam = FD->getParamDecl(NumParams - 1);
+ if (const auto *PET = dyn_cast<PackExpansionType>(LastParam->getType())) {
+ // ... of the type T&&... or T...
+ const auto BaseType = PET->getPattern().getNonReferenceType();
+ if (const auto *TTPT =
+ dyn_cast<TemplateTypeParmType>(BaseType.getTypePtr())) {
+ // ... whose template parameter comes from the function directly
+ if (FT->getTemplateParameters()->getDepth() == TTPT->getDepth()) {
+ return true;
+ }
+ }
+ }
+ }
+ return false;
+ }
+
bool shouldSkipFunctionBody(Decl *D) override {
- // Generally we skip function bodies in preambles for speed.
- // We can make exceptions for functions that are cheap to parse and
- // instantiate, widely used, and valuable (e.g. commonly produce errors).
- if (const auto *FT = llvm::dyn_cast<clang::FunctionTemplateDecl>(D)) {
- if (const auto *II = FT->getDeclName().getAsIdentifierInfo())
- // std::make_unique is trivial, and we diagnose bad constructor calls.
- if (II->isStr("make_unique") && FT->isInStdNamespace())
+ // Usually we don't need to look inside the bodies of header functions
+ // to understand the program. However when forwarding function like
+ // emplace() forward their arguments to some other function, the
+ // interesting overload resolution happens inside the forwarding
+ // function's body. To provide more meaningful diagnostics,
+ // code completion, and parameter hints we should parse (and later
+ // instantiate) the bodies.
+ if (auto *FT = llvm::dyn_cast<clang::FunctionTemplateDecl>(D)) {
+ if (ParseForwardingFunctions) {
+ // Don't skip parsing the body if it looks like a forwarding function
+ if (isLikelyForwardingFunction(FT))
return false;
+ } else {
+ // By default, only take care of make_unique
+ // std::make_unique is trivial, and we diagnose bad constructor calls.
+ if (const auto *II = FT->getDeclName().getAsIdentifierInfo()) {
+ if (II->isStr("make_unique") && FT->isInStdNamespace())
+ return false;
+ }
+ }
}
return true;
}
@@ -161,6 +195,7 @@
const clang::LangOptions *LangOpts = nullptr;
const SourceManager *SourceMgr = nullptr;
PreambleBuildStats *Stats;
+ bool ParseForwardingFunctions;
std::function<void(CompilerInstance &)> BeforeExecuteCallback;
};
@@ -483,11 +518,13 @@
// to read back. We rely on dynamic index for the comments instead.
CI.getPreprocessorOpts().WriteCommentListToPCH = false;
- CppFilePreambleCallbacks CapturedInfo(FileName, PreambleCallback, Stats,
- [&ASTListeners](CompilerInstance &CI) {
- for (const auto &L : ASTListeners)
- L->beforeExecute(CI);
- });
+ CppFilePreambleCallbacks CapturedInfo(
+ FileName, PreambleCallback, Stats,
+ Inputs.Opts.PreambleParseForwardingFunctions,
+ [&ASTListeners](CompilerInstance &CI) {
+ for (const auto &L : ASTListeners)
+ L->beforeExecute(CI);
+ });
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
llvm::SmallString<32> AbsFileName(FileName);
VFS->makeAbsolute(AbsFileName);
Index: clang-tools-extra/clangd/Compiler.h
===================================================================
--- clang-tools-extra/clangd/Compiler.h
+++ clang-tools-extra/clangd/Compiler.h
@@ -39,7 +39,7 @@
// Options to run clang e.g. when parsing AST.
struct ParseOptions {
- // (empty at present, formerly controlled recovery AST, include-fixer etc)
+ bool PreambleParseForwardingFunctions = false;
};
/// Information required to run clang, e.g. to parse AST or do code completion.
Index: clang-tools-extra/clangd/ClangdServer.h
===================================================================
--- clang-tools-extra/clangd/ClangdServer.h
+++ clang-tools-extra/clangd/ClangdServer.h
@@ -164,6 +164,9 @@
/// If true, use the dirty buffer contents when building Preambles.
bool UseDirtyHeaders = false;
+ // If true, parse emplace-like functions in the preamble.
+ bool PreambleParseForwardingFunctions = false;
+
explicit operator TUScheduler::Options() const;
};
// Sensible default options for use in tests.
@@ -416,6 +419,8 @@
bool UseDirtyHeaders = false;
+ bool PreambleParseForwardingFunctions = false;
+
// GUARDED_BY(CachedCompletionFuzzyFindRequestMutex)
llvm::StringMap<llvm::Optional<FuzzyFindRequest>>
CachedCompletionFuzzyFindRequestByFile;
Index: clang-tools-extra/clangd/ClangdServer.cpp
===================================================================
--- clang-tools-extra/clangd/ClangdServer.cpp
+++ clang-tools-extra/clangd/ClangdServer.cpp
@@ -148,7 +148,9 @@
: FeatureModules(Opts.FeatureModules), CDB(CDB), TFS(TFS),
DynamicIdx(Opts.BuildDynamicSymbolIndex ? new FileIndex() : nullptr),
ClangTidyProvider(Opts.ClangTidyProvider),
- UseDirtyHeaders(Opts.UseDirtyHeaders), WorkspaceRoot(Opts.WorkspaceRoot),
+ UseDirtyHeaders(Opts.UseDirtyHeaders),
+ PreambleParseForwardingFunctions(Opts.PreambleParseForwardingFunctions),
+ WorkspaceRoot(Opts.WorkspaceRoot),
Transient(Opts.ImplicitCancellation ? TUScheduler::InvalidateOnUpdate
: TUScheduler::NoInvalidation),
DirtyFS(std::make_unique<DraftStoreFS>(TFS, DraftMgr)) {
@@ -217,6 +219,7 @@
WantDiagnostics WantDiags, bool ForceRebuild) {
std::string ActualVersion = DraftMgr.addDraft(File, Version, Contents);
ParseOptions Opts;
+ Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions;
// Compile command is set asynchronously during update, as it can be slow.
ParseInputs Inputs;
@@ -910,7 +913,7 @@
// It's safe to pass in the TU, as dumpAST() does not
// deserialize the preamble.
auto Node = DynTypedNode::create(
- *Inputs->AST.getASTContext().getTranslationUnitDecl());
+ *Inputs->AST.getASTContext().getTranslationUnitDecl());
return CB(dumpAST(Node, Inputs->AST.getTokens(),
Inputs->AST.getASTContext()));
}
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
