[PATCH] D38208: Add support for remembering origins to ExternalASTMerger
This revision was automatically updated to reflect the committed changes.
Closed by commit rL314336: Add support for remembering origins to
ExternalASTMerger (authored by spyffe).
Changed prior to commit:
https://reviews.llvm.org/D38208?vs=116680&id=116868#toc
Repository:
rL LLVM
https://reviews.llvm.org/D38208
Files:
cfe/trunk/include/clang/AST/ExternalASTMerger.h
cfe/trunk/lib/AST/ExternalASTMerger.cpp
cfe/trunk/lib/Sema/SemaType.cpp
cfe/trunk/test/Import/extern-c-function/Inputs/F.cpp
cfe/trunk/test/Import/extern-c-function/test.cpp
cfe/trunk/test/Import/forward-declared-objc-class/Inputs/S1.m
cfe/trunk/test/Import/forward-declared-objc-class/Inputs/S2.m
cfe/trunk/test/Import/forward-declared-objc-class/Inputs/S3.m
cfe/trunk/test/Import/forward-declared-objc-class/test.m
cfe/trunk/test/Import/forward-declared-struct/Inputs/S3.c
cfe/trunk/test/Import/forward-declared-struct/test.c
cfe/trunk/test/Import/local-struct-use-origins/Inputs/Callee.cpp
cfe/trunk/test/Import/local-struct-use-origins/test.cpp
cfe/trunk/test/Import/local-struct/test.cpp
cfe/trunk/test/Import/objc-definitions-in-expression/Inputs/S.m
cfe/trunk/test/Import/objc-definitions-in-expression/test.m
cfe/trunk/test/Import/struct-and-var/Inputs/S1.cpp
cfe/trunk/test/Import/struct-and-var/Inputs/S2.cpp
cfe/trunk/test/Import/struct-and-var/test.cpp
cfe/trunk/test/Import/template/Inputs/T.cpp
cfe/trunk/test/Import/template/test.cpp
cfe/trunk/tools/clang-import-test/clang-import-test.cpp
Index: cfe/trunk/lib/AST/ExternalASTMerger.cpp
===
--- cfe/trunk/lib/AST/ExternalASTMerger.cpp
+++ cfe/trunk/lib/AST/ExternalASTMerger.cpp
@@ -14,6 +14,7 @@
#include "clang/AST/ASTContext.h"
#include "clang/AST/Decl.h"
+#include "clang/AST/DeclCXX.h"
#include "clang/AST/DeclObjC.h"
#include "clang/AST/ExternalASTMerger.h"
@@ -32,29 +33,18 @@
typedef std::pair, ASTImporter *> Candidate;
-class LazyASTImporter : public ASTImporter {
-public:
- LazyASTImporter(ASTContext &ToContext, FileManager &ToFileManager,
- ASTContext &FromContext, FileManager &FromFileManager)
- : ASTImporter(ToContext, ToFileManager, FromContext, FromFileManager,
-/*MinimalImport=*/true) {}
- Decl *Imported(Decl *From, Decl *To) override {
-if (auto ToTag = dyn_cast(To)) {
- ToTag->setHasExternalLexicalStorage();
- ToTag->setMustBuildLookupTable();
-} else if (auto ToNamespace = dyn_cast(To)) {
- ToNamespace->setHasExternalVisibleStorage();
-} else if (auto ToContainer = dyn_cast(To)) {
- ToContainer->setHasExternalLexicalStorage();
- ToContainer->setMustBuildLookupTable();
-}
-return ASTImporter::Imported(From, To);
- }
-};
+/// For the given DC, return the DC that is safe to perform lookups on. This is
+/// the DC we actually want to work with most of the time.
+const DeclContext *CanonicalizeDC(const DeclContext *DC) {
+ if (isa(DC))
+return DC->getRedeclContext();
+ return DC;
+}
Source
LookupSameContext(Source SourceTU, const DeclContext *DC,
ASTImporter &ReverseImporter) {
+ DC = CanonicalizeDC(DC);
if (DC->isTranslationUnit()) {
return SourceTU;
}
@@ -64,101 +54,328 @@
// If we couldn't find the parent DC in this TranslationUnit, give up.
return nullptr;
}
- auto ND = cast(DC);
+ auto *ND = cast(DC);
DeclarationName Name = ND->getDeclName();
Source SourceName = ReverseImporter.Import(Name);
DeclContext::lookup_result SearchResult =
SourceParentDC.get()->lookup(SourceName.get());
size_t SearchResultSize = SearchResult.size();
- // Handle multiple candidates once we have a test for it.
- // This may turn up when we import template specializations correctly.
- assert(SearchResultSize < 2);
- if (SearchResultSize == 0) {
-// couldn't find the name, so we have to give up
+ if (SearchResultSize == 0 || SearchResultSize > 1) {
+// There are two cases here. First, we might not find the name.
+// We might also find multiple copies, in which case we have no
+// guarantee that the one we wanted is the one we pick. (E.g.,
+// if we have two specializations of the same template it is
+// very hard to determine which is the one you want.)
+//
+// The Origins map fixes this problem by allowing the origin to be
+// explicitly recorded, so we trigger that recording by returning
+// nothing (rather than a possibly-inaccurate guess) here.
return nullptr;
} else {
NamedDecl *SearchResultDecl = SearchResult[0];
-return dyn_cast(SearchResultDecl);
+if (isa(SearchResultDecl) &&
+SearchResultDecl->getKind() == DC->getDeclKind())
+ return cast(SearchResultDecl)->getPrimaryContext();
+return nullptr; // This type of lookup is unsupported
}
}
-bool IsForwardDeclaration(Decl *D) {
- if (auto TD = dyn_cast(D)) {
-return !TD->is
[PATCH] D38208: Add support for remembering origins to ExternalASTMerger
bruno accepted this revision. bruno added a comment. This revision is now accepted and ready to land. LGTM Repository: rL LLVM https://reviews.llvm.org/D38208 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38208: Add support for remembering origins to ExternalASTMerger
spyffe updated this revision to Diff 116680.
spyffe added a comment.
Updates and small refactors requested by Bruno.
Repository:
rL LLVM
https://reviews.llvm.org/D38208
Files:
include/clang/AST/ExternalASTMerger.h
lib/AST/ExternalASTMerger.cpp
lib/Sema/SemaType.cpp
test/Import/extern-c-function/Inputs/F.cpp
test/Import/extern-c-function/test.cpp
test/Import/forward-declared-objc-class/Inputs/S1.m
test/Import/forward-declared-objc-class/Inputs/S2.m
test/Import/forward-declared-objc-class/Inputs/S3.m
test/Import/forward-declared-objc-class/test.m
test/Import/forward-declared-struct/Inputs/S3.c
test/Import/forward-declared-struct/test.c
test/Import/local-struct-use-origins/Inputs/Callee.cpp
test/Import/local-struct-use-origins/test.cpp
test/Import/local-struct/test.cpp
test/Import/objc-definitions-in-expression/Inputs/S.m
test/Import/objc-definitions-in-expression/test.m
test/Import/struct-and-var/Inputs/S1.cpp
test/Import/struct-and-var/Inputs/S2.cpp
test/Import/struct-and-var/test.cpp
test/Import/template/Inputs/T.cpp
test/Import/template/test.cpp
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -48,8 +48,12 @@
static llvm::cl::opt
Direct("direct", llvm::cl::Optional,
- llvm::cl::desc("Use the parsed declarations without indirection"));
+ llvm::cl::desc("Use the parsed declarations without indirection"));
+static llvm::cl::opt
+UseOrigins("use-origins", llvm::cl::Optional,
+ llvm::cl::desc("Use DeclContext origin information for more accurate lookups"));
+
static llvm::cl::list
ClangArgs("Xcc", llvm::cl::ZeroOrMore,
llvm::cl::desc("Argument to pass to the CompilerInvocation"),
@@ -60,13 +64,11 @@
llvm::cl::desc("The language to parse (default: c++)"),
llvm::cl::init("c++"));
-static llvm::cl::opt
-DumpAST("dump-ast", llvm::cl::init(false),
-llvm::cl::desc("Dump combined AST"));
+static llvm::cl::opt DumpAST("dump-ast", llvm::cl::init(false),
+ llvm::cl::desc("Dump combined AST"));
-static llvm::cl::opt
-DumpIR("dump-ir", llvm::cl::init(false),
-llvm::cl::desc("Dump IR from final parse"));
+static llvm::cl::opt DumpIR("dump-ir", llvm::cl::init(false),
+ llvm::cl::desc("Dump IR from final parse"));
namespace init_convenience {
class TestDiagnosticConsumer : public DiagnosticConsumer {
@@ -154,8 +156,7 @@
}
};
-std::unique_ptr
-BuildCompilerInstance() {
+std::unique_ptr BuildCompilerInstance() {
auto Ins = llvm::make_unique();
auto DC = llvm::make_unique();
const bool ShouldOwnClient = true;
@@ -227,29 +228,54 @@
} // end namespace
namespace {
-
-void AddExternalSource(
-CompilerInstance &CI,
-llvm::ArrayRef> Imports) {
- ExternalASTMerger::ImporterEndpoint Target({CI.getASTContext(), CI.getFileManager()});
- llvm::SmallVector Sources;
- for (const std::unique_ptr &CI : Imports) {
-Sources.push_back({CI->getASTContext(), CI->getFileManager()});
+
+/// A container for a CompilerInstance (possibly with an ExternalASTMerger
+/// attached to its ASTContext).
+///
+/// Provides an accessor for the DeclContext origins associated with the
+/// ExternalASTMerger (or an empty list of origins if no ExternalASTMerger is
+/// attached).
+///
+/// This is the main unit of parsed source code maintained by clang-import-test.
+struct CIAndOrigins {
+ using OriginMap = clang::ExternalASTMerger::OriginMap;
+ std::unique_ptr CI;
+
+ ASTContext &getASTContext() { return CI->getASTContext(); }
+ FileManager &getFileManager() { return CI->getFileManager(); }
+ const OriginMap &getOriginMap() {
+static const OriginMap EmptyOriginMap;
+if (ExternalASTSource *Source = CI->getASTContext().getExternalSource())
+ return static_cast(Source)->GetOrigins();
+return EmptyOriginMap;
}
+ DiagnosticConsumer &getDiagnosticClient() {
+return CI->getDiagnosticClient();
+ }
+ CompilerInstance &getCompilerInstance() { return *CI; }
+};
+
+void AddExternalSource(CIAndOrigins &CI,
+ llvm::MutableArrayRef Imports) {
+ ExternalASTMerger::ImporterTarget Target(
+ {CI.getASTContext(), CI.getFileManager()});
+ llvm::SmallVector Sources;
+ for (CIAndOrigins &Import : Imports)
+Sources.push_back(
+{Import.getASTContext(), Import.getFileManager(), Import.getOriginMap()});
auto ES = llvm::make_unique(Target, Sources);
CI.getASTContext().setExternalSource(ES.release());
CI.getASTContext().getTranslationUnitDecl()->setHasExternalVisibleStorage();
}
-std::unique_ptr BuildIndirect(std::unique_ptr &CI) {
- std::unique_ptr IndirectCI =
- init_convenience::BuildCompilerInstance();
+CIAndOrigins B
Re: [PATCH] D38208: Add support for remembering origins to ExternalASTMerger
Updated the diff. Unfortunately reviews.llvm.org is down, so I'll try
updating that page in the morning.
Sean
On 9/25/17 5:04 AM, Bruno Cardoso Lopes via Phabricator wrote:
bruno added inline comments.
Comment at: lib/AST/ExternalASTMerger.cpp:79
+ return cast(SearchResultDecl)->getPrimaryContext();
+else
+ return nullptr; // This type of lookup is unsupported
No need for `else` here.
Comment at: lib/AST/ExternalASTMerger.cpp:173
+ASTImporter &ExternalASTMerger::ImporterForOrigin(ASTContext &OriginContext) {
+ for (const std::unique_ptr &I : Importers) {
+if (&I->getFromContext() == &OriginContext)
No need for curly braces here.
Comment at: lib/AST/ExternalASTMerger.cpp:189
+bool ExternalASTMerger::HasImporterForOrigin(ASTContext &OriginContext) {
+ for (const std::unique_ptr &I : Importers) {
+if (&I->getFromContext() == &OriginContext)
No need for curly braces here.
Comment at: lib/AST/ExternalASTMerger.cpp:289
+ LookupSameContext(Origin.AST->getTranslationUnitDecl(), ToDC, Reverse);
+ if (!FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC)) {
+if (LoggingEnabled())
You can probably simplify this to something like:
```
bool RecordOrigin = !FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC);
if (RecordOrigin)
RecordOriginImpl(ToDC, Origin, Importer);
if (LoggingEnabled()) {
logs() << "(ExternalASTMerger*)" << (void*)this
<< " decided";
if (!RecordOrigin)
logs() << " NOT";
logs() << " to record origin (DeclContext*)" << (void*)Origin.DC
<< ", (ASTContext*)" << (void*)&Origin.AST
<< "\n";
}
```
Comment at: lib/AST/ExternalASTMerger.cpp:380
+
+ if (Candidates.empty()) {
+return false;
No need for curly braces here.
Comment at: tools/clang-import-test/clang-import-test.cpp:329
CG.GetModule()->print(llvm::outs(), nullptr);
- if (CI->getDiagnosticClient().getNumErrors()) {
+ if (CI.getDiagnosticClient().getNumErrors()) {
return llvm::make_error(
No need for curly braces here
Repository:
rL LLVM
https://reviews.llvm.org/D38208
Index: include/clang/AST/ExternalASTMerger.h
===
--- include/clang/AST/ExternalASTMerger.h (revision 313996)
+++ include/clang/AST/ExternalASTMerger.h (working copy)
@@ -1,51 +1,176 @@
//===--- ExternalASTMerger.h - Merging External AST Interface ---*- C++
-*-===//
//
// The LLVM Compiler Infrastructure
//
// This file is distributed under the University of Illinois Open Source
// License. See LICENSE.TXT for details.
//
//===--===//
//
// This file declares the ExternalASTMerger, which vends a combination of ASTs
// from several different ASTContext/FileManager pairs
//
//===--===//
#ifndef LLVM_CLANG_AST_EXTERNALASTMERGER_H
#define LLVM_CLANG_AST_EXTERNALASTMERGER_H
#include "clang/AST/ASTImporter.h"
#include "clang/AST/ExternalASTSource.h"
+#include "llvm/Support/raw_ostream.h"
namespace clang {
+/// ExternalASTSource implementation that merges information from several
+/// ASTContexts.
+///
+/// ExtermalASTMerger maintains a vector of ASTImporters that it uses to import
+/// (potentially incomplete) Decls and DeclContexts from the source ASTContexts
+/// in response to ExternalASTSource API calls.
+///
+/// When lookup occurs in the resulting imported DeclContexts, the original
+/// DeclContexts need to be queried. Roughly, there are three cases here:
+///
+/// - The DeclContext of origin can be found by simple name lookup. In this
+/// case, no additional state is required.
+///
+/// - The DeclContext of origin is different from what would be found by name
+/// lookup. In this case, Origins contains an entry overriding lookup and
+/// specifying the correct pair of DeclContext/ASTContext.
+///
+/// - The DeclContext of origin was determined by another ExterenalASTMerger.
+/// (This is possible when the source ASTContext for one of the Importers has
+/// its own ExternalASTMerger). The origin must be properly forwarded in
this
+/// case.
+///
+/// ExternalASTMerger's job is to maintain the data structures necessary to
+/// allow this. The data structures themselves can be extracted (read-only)
and
+/// copied for re-use.
class ExternalASTMerger : public ExternalASTSource {
public:
- struct ImporterPair {
-std::unique_ptr Forward;
-std::unique_ptr Reverse;
+ /// A single origin for a DeclContext. Unlike Decls, DeclContexts do
+ /// not allow their containing ASTContext to be determined in all cases.
+ struct DCOrigin {
+DeclContext
[PATCH] D38208: Add support for remembering origins to ExternalASTMerger
bruno added inline comments.
Comment at: lib/AST/ExternalASTMerger.cpp:79
+ return cast(SearchResultDecl)->getPrimaryContext();
+else
+ return nullptr; // This type of lookup is unsupported
No need for `else` here.
Comment at: lib/AST/ExternalASTMerger.cpp:173
+ASTImporter &ExternalASTMerger::ImporterForOrigin(ASTContext &OriginContext) {
+ for (const std::unique_ptr &I : Importers) {
+if (&I->getFromContext() == &OriginContext)
No need for curly braces here.
Comment at: lib/AST/ExternalASTMerger.cpp:189
+bool ExternalASTMerger::HasImporterForOrigin(ASTContext &OriginContext) {
+ for (const std::unique_ptr &I : Importers) {
+if (&I->getFromContext() == &OriginContext)
No need for curly braces here.
Comment at: lib/AST/ExternalASTMerger.cpp:289
+ LookupSameContext(Origin.AST->getTranslationUnitDecl(), ToDC, Reverse);
+ if (!FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC)) {
+if (LoggingEnabled())
You can probably simplify this to something like:
```
bool RecordOrigin = !FoundFromDC || !IsSameDC(FoundFromDC.get(), Origin.DC);
if (RecordOrigin)
RecordOriginImpl(ToDC, Origin, Importer);
if (LoggingEnabled()) {
logs() << "(ExternalASTMerger*)" << (void*)this
<< " decided";
if (!RecordOrigin)
logs() << " NOT";
logs() << " to record origin (DeclContext*)" << (void*)Origin.DC
<< ", (ASTContext*)" << (void*)&Origin.AST
<< "\n";
}
```
Comment at: lib/AST/ExternalASTMerger.cpp:380
+
+ if (Candidates.empty()) {
+return false;
No need for curly braces here.
Comment at: tools/clang-import-test/clang-import-test.cpp:329
CG.GetModule()->print(llvm::outs(), nullptr);
- if (CI->getDiagnosticClient().getNumErrors()) {
+ if (CI.getDiagnosticClient().getNumErrors()) {
return llvm::make_error(
No need for curly braces here
Repository:
rL LLVM
https://reviews.llvm.org/D38208
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D38208: Add support for remembering origins to ExternalASTMerger
spyffe created this revision.
spyffe added a project: clang.
At @bruno 's request, recreating this with cfe-commits as a subscriber. The
original is https://reviews.llvm.org/D36589
`ExternalASTMerger` has hitherto relied on being able to look up any Decl
through its named `DeclContext` chain. This works for many cases, but causes
problems for function-local structs, which cannot be looked up in their
containing `FunctionDecl`. An example case is
void f() {
{ struct S { int a; }; }
{ struct S { bool b; }; }
}
It is not possible to lookup either of the two `S`es individually (or even to
provide enough information to disambiguate) after parsing is over; and there is
typically no need to, since they are invisible to the outside world.
However, ExternalASTMerger needs to be able to complete either S on demand.
This led to an `XFAIL` on test/Import/local-struct, which this patch removes.
The way the patch works is:
- It defines a new data structure, `ExternalASTMerger::OriginMap`, which
clients are expected to maintain (default-constructing if the origin does not
have an `ExternalASTMerger` servicing it)
- As `DeclContext`s are imported, if they cannot be looked up by name they are
placed in the `OriginMap`. This allows `ExternalASTMerger` to complete them
later if necessary.
- As `DeclContext`s are imported from an origin that already has its own
`OriginMap`, the origins are forwarded – but only for those `DeclContext`s that
are actually used. This keeps the amount of stored data minimal.
Repository:
rL LLVM
https://reviews.llvm.org/D38208
Files:
include/clang/AST/ExternalASTMerger.h
lib/AST/ExternalASTMerger.cpp
lib/Sema/SemaType.cpp
test/Import/extern-c-function/Inputs/F.cpp
test/Import/extern-c-function/test.cpp
test/Import/forward-declared-objc-class/Inputs/S1.m
test/Import/forward-declared-objc-class/Inputs/S2.m
test/Import/forward-declared-objc-class/Inputs/S3.m
test/Import/forward-declared-objc-class/test.m
test/Import/forward-declared-struct/Inputs/S3.c
test/Import/forward-declared-struct/test.c
test/Import/local-struct-use-origins/Inputs/Callee.cpp
test/Import/local-struct-use-origins/test.cpp
test/Import/local-struct/test.cpp
test/Import/objc-definitions-in-expression/Inputs/S.m
test/Import/objc-definitions-in-expression/test.m
test/Import/struct-and-var/Inputs/S1.cpp
test/Import/struct-and-var/Inputs/S2.cpp
test/Import/struct-and-var/test.cpp
test/Import/template/Inputs/T.cpp
test/Import/template/test.cpp
tools/clang-import-test/clang-import-test.cpp
Index: tools/clang-import-test/clang-import-test.cpp
===
--- tools/clang-import-test/clang-import-test.cpp
+++ tools/clang-import-test/clang-import-test.cpp
@@ -48,8 +48,12 @@
static llvm::cl::opt
Direct("direct", llvm::cl::Optional,
- llvm::cl::desc("Use the parsed declarations without indirection"));
+ llvm::cl::desc("Use the parsed declarations without indirection"));
+static llvm::cl::opt
+UseOrigins("use-origins", llvm::cl::Optional,
+ llvm::cl::desc("Use DeclContext origin information for more accurate lookups"));
+
static llvm::cl::list
ClangArgs("Xcc", llvm::cl::ZeroOrMore,
llvm::cl::desc("Argument to pass to the CompilerInvocation"),
@@ -60,13 +64,11 @@
llvm::cl::desc("The language to parse (default: c++)"),
llvm::cl::init("c++"));
-static llvm::cl::opt
-DumpAST("dump-ast", llvm::cl::init(false),
-llvm::cl::desc("Dump combined AST"));
+static llvm::cl::opt DumpAST("dump-ast", llvm::cl::init(false),
+ llvm::cl::desc("Dump combined AST"));
-static llvm::cl::opt
-DumpIR("dump-ir", llvm::cl::init(false),
-llvm::cl::desc("Dump IR from final parse"));
+static llvm::cl::opt DumpIR("dump-ir", llvm::cl::init(false),
+ llvm::cl::desc("Dump IR from final parse"));
namespace init_convenience {
class TestDiagnosticConsumer : public DiagnosticConsumer {
@@ -154,8 +156,7 @@
}
};
-std::unique_ptr
-BuildCompilerInstance() {
+std::unique_ptr BuildCompilerInstance() {
auto Ins = llvm::make_unique();
auto DC = llvm::make_unique();
const bool ShouldOwnClient = true;
@@ -227,29 +228,54 @@
} // end namespace
namespace {
-
-void AddExternalSource(
-CompilerInstance &CI,
-llvm::ArrayRef> Imports) {
- ExternalASTMerger::ImporterEndpoint Target({CI.getASTContext(), CI.getFileManager()});
- llvm::SmallVector Sources;
- for (const std::unique_ptr &CI : Imports) {
-Sources.push_back({CI->getASTContext(), CI->getFileManager()});
+
+/// A container for a CompilerInstance (possibly with an ExternalASTMerger
+/// attached to its ASTContext).
+///
+/// Provides an accessor for the DeclContext origins associated with the
+/// ExternalASTMerger (or an empty list of origins if no ExternalASTMerger is
+/// attached).
+/
