[PATCH] D48628: [AST] Structural equivalence of methods

[Aleksei Sidorin via Phabricator via cfe-commits]

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