Skip to content
This repository has been archived by the owner on Mar 30, 2021. It is now read-only.

Added check for scope in structural equivalence (experimental). #629

Open
wants to merge 3 commits into
base: ctu-clang7
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
127 changes: 127 additions & 0 deletions lib/AST/ASTStructuralEquivalence.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,111 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
const TemplateArgument &Arg1,
const TemplateArgument &Arg2);
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
const TemplateArgumentList &ArgL1,
const TemplateArgumentList &ArgL2);
static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
NestedNameSpecifier *NNS1,
NestedNameSpecifier *NNS2);
static bool IsStructurallyEquivalent(const IdentifierInfo *Name1,
const IdentifierInfo *Name2);
static bool IsStructurallyEquivalent(const DeclarationName Name1,
const DeclarationName Name2);

using ContextVector = llvm::SmallVector<const DeclContext *, 4>;

static void GetContextsForEquivalenceCheck(ContextVector &Contexts,
const NamedDecl *ND) {
const DeclContext *Ctx = ND->getDeclContext();

// For ObjC methods, look through categories and use the interface as context.
if (auto *MD = dyn_cast<ObjCMethodDecl>(ND))
if (auto *ID = MD->getClassInterface())
Ctx = ID;

// Collect named contexts.
// Function contexts are ignored, this behavior is enough for ASTImporter.
// FIXME: Add assertion if different (non equivalent) functions are
// encountered on the paths (except the first)?
while (Ctx) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems dangerous, what is the parent of a TranslationUnitDecl? Perhaps another guard is needed?
while (Ctx && !Ctx->isTranslationUnit())

if (isa<NamedDecl>(Ctx) && !isa<FunctionDecl>(Ctx))
Contexts.push_back(Ctx);
Ctx = Ctx->getParent();
}
}

static bool IsEquivalentDeclContext(StructuralEquivalenceContext &Context,
const DeclContext *DC1,
const DeclContext *DC2) {
if (const auto *Spec1 = dyn_cast<ClassTemplateSpecializationDecl>(DC1)) {
const auto *Spec2 = dyn_cast<ClassTemplateSpecializationDecl>(DC2);
if (!Spec2)
return false;
if (!IsStructurallyEquivalent(Spec1->getDeclName(), Spec2->getDeclName()))
return false;
if (!IsStructurallyEquivalent(Context, Spec1->getTemplateArgs(),
Spec2->getTemplateArgs()))
return false;
} else if (const auto *ND1 = dyn_cast<NamespaceDecl>(DC1)) {
const auto *ND2 = dyn_cast<NamespaceDecl>(DC2);
if (!ND2)
return false;
if (ND1->isAnonymousNamespace() != ND2->isAnonymousNamespace())
return false;
if (ND1->isInline() != ND2->isInline())
return false;
if (ND1->isAnonymousNamespace() || ND1->isInlineNamespace())
return true;
if (!IsStructurallyEquivalent(ND1->getDeclName(), ND2->getDeclName()))
return false;
} else if (const auto *RD1 = dyn_cast<RecordDecl>(DC1)) {
const auto *RD2 = dyn_cast<RecordDecl>(DC2);
if (!RD2)
return false;
if (!IsStructurallyEquivalent(RD1->getDeclName(), RD2->getDeclName()))
return false;
} else if (const auto *ED1 = dyn_cast<EnumDecl>(DC1)) {
const auto *ED2 = dyn_cast<EnumDecl>(DC2);
if (!ED2)
return false;
if (ED1->isScoped() != ED2->isScoped())
return false;
} else if (const auto *ND1 = dyn_cast<NamedDecl>(DC1)) {
const auto *ND2 = cast<NamedDecl>(DC2);
if (!IsStructurallyEquivalent(ND1->getDeclName(), ND2->getDeclName()))
return false;
} else
llvm_unreachable("Context should be NamedDecl.");

return true;
}

/// This function checks for equivalent "context" (scope) of an entity.
/// This should check if the scope of two names is equivalent.
/// Functions are excluded from this scope check because there is no use case
/// when entities from different functions are compared.
static bool IsEquivalentContext(StructuralEquivalenceContext &Context,
const NamedDecl *ND1, const NamedDecl *ND2) {
ContextVector Contexts1, Contexts2;
GetContextsForEquivalenceCheck(Contexts1, ND1);
GetContextsForEquivalenceCheck(Contexts2, ND2);

auto DC2I = Contexts2.begin();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a check before to see whether the sizes are equal and then an early return if not would make it simpler.

for (const DeclContext *DC1 : Contexts1) {
if (DC2I == Contexts2.end())
return false;
const DeclContext *DC2 = *DC2I;
++DC2I;

if (!IsEquivalentDeclContext(Context, DC1, DC2))
return false;
}

if (DC2I != Contexts2.end())
return false;

return true;
}

static bool IsStructurallyEquivalent(const DeclarationName Name1,
const DeclarationName Name2) {
Expand Down Expand Up @@ -338,6 +438,23 @@ static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
llvm_unreachable("Invalid template argument kind");
}

/// Determine whether two template argument lists are equivalent.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this hunk related?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see you use it when comparing the contexts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function is used in IsEquivalentContext.

static bool IsStructurallyEquivalent(StructuralEquivalenceContext &Context,
const TemplateArgumentList &ArgL1,
const TemplateArgumentList &ArgL2) {
if (ArgL1.size() != ArgL2.size()) {
if (Context.Complain) {
}
return false;
}

for (unsigned I = 0, N = ArgL1.size(); I != N; ++I)
if (!IsStructurallyEquivalent(Context, ArgL1.get(I), ArgL2.get(I)))
return false;

return true;
}

/// Determine structural equivalence for the common part of array
/// types.
static bool IsArrayStructurallyEquivalent(StructuralEquivalenceContext &Context,
Expand Down Expand Up @@ -1633,6 +1750,8 @@ unsigned StructuralEquivalenceContext::getApplicableDiagnostic(
return diag::warn_odr_parameter_pack_non_pack;
case diag::err_odr_non_type_parameter_type_inconsistent:
return diag::warn_odr_non_type_parameter_type_inconsistent;
default:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is probably not related too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to make a build warning go away (I have another one in ASTImporterTest.cpp, probably a new PR can be created to correct this).

llvm_unreachable("Unknown diagnostic type.");
}
}

Expand Down Expand Up @@ -1674,6 +1793,14 @@ bool StructuralEquivalenceContext::CheckCommonEquivalence(Decl *D1, Decl *D2) {

// FIXME: Move check for identifier names into this function.

if (auto *ND1 = dyn_cast<NamedDecl>(D1)) {
auto *ND2 = dyn_cast<NamedDecl>(D2);
if (!ND2)
return false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if here we would just get the two DeclContexts from D1 and D2 and we would compare them by either NamedDecl::getQualifiedNameAsString or by index::generateUSRForDecl?
The USR seems more precise and both seems to be slow.
About the slowness we could add yet another local cache to the StructuralEquivalenceContext class.

How is it different than what we had previously in #624 ?
This time we would compare only the containing scopes by string and not the Decls themselves.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I am saying here is just an alternative solution to consider, not necessarily better ...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to compare only scope, not the decl name itself (that is done already). Probably this was the reason for some test failures when the qualified name was used.

if (!IsEquivalentContext(*this, ND1, ND2))
return false;
}

return true;
}

Expand Down
9 changes: 9 additions & 0 deletions unittests/AST/StructuralEquivalenceTest.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -403,6 +403,15 @@ TEST_F(StructuralEquivalenceFunctionTest,
EXPECT_TRUE(testStructuralMatch(t));
}

TEST_F(StructuralEquivalenceFunctionTest,
ParameterInDifferentScopeShouldBeInequal) {
auto t = makeNamedDecls(
"namespace A { class X; } void foo(A::X &x);",
"namespace B { class X; } void foo(B::X &x);",
Lang_CXX);
EXPECT_FALSE(testStructuralMatch(t));
}

struct StructuralEquivalenceCXXMethodTest : StructuralEquivalenceTest {
};

Expand Down