a.sidorin added a comment.
Hello Gabor,
This patch is really cool. Unfortunately, right now I don't have enough time
for reviewing large patches in a single review. Are you OK with incremental
review?
================
Comment at: lib/AST/ASTImporter.cpp:75
+ template <class T>
+ llvm::SmallVector<Decl*, 2>
+ getCanonicalForwardRedeclChain(Redeclarable<T>* D) {
----------------
We can drop `llvm::` prefix for SmallVectors.
================
Comment at: lib/AST/ASTImporter.cpp:78
+ llvm::SmallVector<Decl*, 2> Redecls;
+ for (auto R : D->getFirstDecl()->redecls()) {
+ if (R != D->getFirstDecl())
----------------
Please keep `*` where possible.
================
Comment at: lib/AST/ASTImporter.cpp:79
+ for (auto R : D->getFirstDecl()->redecls()) {
+ if (R != D->getFirstDecl())
+ Redecls.push_back(R);
----------------
Does this code mean that we can get R == getFirstDecl() in the middle of the
import chain?
================
Comment at: lib/AST/ASTImporter.cpp:2340
+ FunctionDecl::TK_FunctionTemplateSpecialization);
+ auto *FTSInfo = FromFD->getTemplateSpecializationInfo();
+ auto *Template = cast_or_null<FunctionTemplateDecl>(
----------------
This code should be unified with `ImportTemplateInformation()` to avoid
duplication.
================
Comment at: lib/AST/ASTImporter.cpp:2363
+ // declarations starting from the canonical decl.
+ for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+ Importer.Import(*RedeclIt);
----------------
Can we just import `getPreviousDecl()`?
================
Comment at: lib/AST/ASTImporter.cpp:2364
+ for (;RedeclIt != Redecls.end() && *RedeclIt != D; ++RedeclIt)
+ Importer.Import(*RedeclIt);
+ assert(*RedeclIt == D);
----------------
Unchecked import result, is it intentional?
================
Comment at: lib/AST/ASTImporter.cpp:2380
+ // If this is a function template specialization, then try to find the same
+ // existing specialization in the "to" context. The localUncachedLookup
+ // below will not find any specialization, but would find the primary
----------------
Double space.
================
Comment at: lib/AST/ASTImporter.cpp:2409
FoundFunction->getType())) {
- // FIXME: Actually try to merge the body and other attributes.
- const FunctionDecl *FromBodyDecl = nullptr;
- D->hasBody(FromBodyDecl);
- if (D == FromBodyDecl && !FoundFunction->hasBody()) {
- // This function is needed to merge completely.
- FoundWithoutBody = FoundFunction;
+ if (D->doesThisDeclarationHaveABody() &&
+ FoundFunction->hasBody())
----------------
Has removed comment become irrelevant? (Same below)
================
Comment at: lib/AST/ASTImporter.cpp:2609
+ // Friend declarations lexical context is the befriending class, but the
+ // semantic context is the enclosing scope of the befriending class.
----------------
declaration's
================
Comment at: lib/AST/ASTImporter.cpp:2612
+ // We want the friend functions to be found in the semantic context by
lookup.
+ // FIXME should we handle this genrically in VisitFriendDecl?
+ // In Other cases when LexicalDC != DC we don't want it to be added,
----------------
generically?
================
Comment at: lib/AST/ASTImporter.cpp:2619
+
+ // Import the rest of the chain. I.e. import all subsequent declarations.
+ for (++RedeclIt; RedeclIt != Redecls.end(); ++RedeclIt)
----------------
So, we're importing a redeclaration. It imports its own redeclarations during
import. This means that after the first redecl is imported, all other redecls
should be imported too. So, do we need to have a loop here?
================
Comment at: test/ASTMerge/class/test.cpp:20
+// CHECK: class1.cpp:36:8: warning: type 'F2' has incompatible definitions in
different translation units
+// CHECK: class1.cpp:39:3: note: friend declared here
----------------
Why did the order change?
================
Comment at: unittests/AST/ASTImporterTest.cpp:1745
+TEST_P(ImportFunctions, ImportDefinitions) {
+ auto Pattern = functionDecl(hasName("f"));
+
----------------
martong wrote:
> balazske wrote:
> > This test imports the definition two times, the second should result in
> > error. The import does not check the function body for similarness.
> > Probably a check can be made for the result of the second import.
> I don't think this should result an error, because the second definition has
> the very same type as the first. And also the body is structurally equivalent
> too, but we do not check that. Perhaps, on a long term we should check the
> body too (?).
>
> I think the importer should work in this way: if there is already a
> definition then just simply use that and omit any subsequent definitions if
> they are the same definitions (or report error if the body is different).
> The key principle I followed is this: we can have arbitrary number of
> prototypes but only one definition.
I think this is a right approach because we have ODR. Unless function
signatures are different, only one possible definition should be used.
This situation is quite different from ASTImporter approach because ASTImporter
checker for declaration compatibility, not definition compatibility.
ASTImporter does not check function bodies for similarity, and I believe it
shouldn't.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1838
+ ASSERT_EQ(DeclCounter<FunctionDecl>().match(ToTU, Pattern), 3u);
+ FunctionDecl* ProtoD = FirstDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
EXPECT_TRUE(!ProtoD->doesThisDeclarationHaveABody());
----------------
Misplaced `*`s appeared. Could you clang-format?
================
Comment at: unittests/AST/ASTImporterTest.cpp:1924
+ ASSERT_EQ(DeclCounter<FunctionDecl>().match(ToTU, Pattern), 2u);
+ EXPECT_TRUE(!ImportedD->doesThisDeclarationHaveABody());
+ auto ToFD = LastDeclMatcher<FunctionDecl>().match(ToTU, Pattern);
----------------
We can replace EXPECT_TRUE with negations with EXPECT_FALSE.
Repository:
rC Clang
https://reviews.llvm.org/D47532
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
