a.sidorin requested changes to this revision.
a.sidorin added a comment.
This revision now requires changes to proceed.
Hi Balázs,
I think that the test changes unrelated to C++ method equivalence should be
moved into a separate patch.
================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:851
+ CXXMethodDecl *Method2) {
+ if (Method1->isStatic() != Method2->isStatic())
+ return false;
----------------
```bool QualifiersEqual = Method1->isStatic() == Method2->isStatic() &&
Method1->isConst() == Method2->isConst() &&...
if (!QualifiersEqual)
return false;
```
================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:873
+
+ if (auto *Constructor1 = dyn_cast<CXXConstructorDecl>(Method1)) {
+ if (auto *Constructor2 = dyn_cast<CXXConstructorDecl>(Method2)) {
----------------
```if (Method1->getStmtKind() != Method2->getStmtKind())
return false;```
So we need to check only one declaration here and below.
================
Comment at: lib/AST/ASTStructuralEquivalence.cpp:892
+
+ if (Method1->isOverloadedOperator() && Method2->isOverloadedOperator()) {
+ if (Method1->getOverloadedOperator() != Method2->getOverloadedOperator())
----------------
These two lines look a bit strange to me. For example, should we return false
if one of the methods is an overloaded operator and other one is not? I guess
these conditions need to be swapped or written like this:
```
if (Method1->getOverloadedOperator() != Method2->getOverloadedOperator())
return false;
const IdentifierInfo *Literal1 = Method1->getLiteralIdentifier();
const IdentifierInfo *Literal2 = Method2->getLiteralIdentifier();
if (!::IsStructurallyEquivalent(Literal1, Literal2))
return false;
```
omitting the first condition.
================
Comment at: unittests/AST/ASTImporterTest.cpp:1558
+TEST_P(ASTImporterTestBase, ImportOfDeclAfterFwdDecl) {
+ Decl *ToD1;
----------------
This test doesn't look related. Could you please move such tests into a
separate patch?They make the review harder.
Repository:
rC Clang
https://reviews.llvm.org/D48628
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
