Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang][AST] fix lack comparison of declRefExpr in ASTStructuralEquivalence #66041

Merged
merged 1 commit into from
Sep 21, 2023

Conversation

mzyKi
Copy link
Contributor

@mzyKi mzyKi commented Sep 12, 2023

Fixes #66047
Before fix,the following testcase expected true. While I think only comparison of declName is not sufficient.Thanks for giving suggestions.

TEST_F(StructuralEquivalenceStmtTest, DeclRefENoEq) {
  std::string Prefix = "enum Test { AAA, BBB };";
  auto t = makeStmts(
      Prefix + "void foo(int i) {if (i > 0) {i = AAA;} else {i = BBB;}}",
      Prefix + "void foo(int i) {if (i > 0) {i = BBB;} else {i = AAA;}}",
      Lang_CXX03, ifStmt());
  EXPECT_FALSE(testStructuralMatch(t)); // EXPECT_TRUE
}

@mzyKi mzyKi requested a review from a team as a code owner September 12, 2023 03:10
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Sep 12, 2023
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

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

Thanks for working on that!
Can you explain why you are comparing declaration only with their name? that seems suspicious to me. Otherwise, I think the intent of the change is correct

clang/lib/AST/ASTStructuralEquivalence.cpp Outdated Show resolved Hide resolved
clang/lib/AST/ASTStructuralEquivalence.cpp Outdated Show resolved Hide resolved
@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch from 1efbe57 to 18039e2 Compare September 12, 2023 08:25
@cor3ntin
Copy link
Contributor

You should edit clang/docs/ReleaseNotes.rst, indicating that #66047 was fixed,
and edit the description of the PR with Fixes #66047 so that it gets closed automatically.

Otherwise LGTM

@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch from 18039e2 to 0ddd5db Compare September 12, 2023 09:03
@mzyKi mzyKi requested a review from cor3ntin September 12, 2023 09:05
@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch from 0ddd5db to fd7138e Compare September 14, 2023 02:59
@mzyKi mzyKi requested a review from cor3ntin September 14, 2023 05:37
@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch 2 times, most recently from c602243 to 011c399 Compare September 15, 2023 10:06
@danix800 danix800 requested a review from a team September 15, 2023 10:08
@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch 2 times, most recently from f74271d to a1635d8 Compare September 15, 2023 10:51
@mzyKi mzyKi changed the title [clang] fix lack comparison of declRefExpr in ASTStructuralEquivalence [clang][AST] fix lack comparison of declRefExpr in ASTStructuralEquivalence Sep 15, 2023
@mzyKi mzyKi requested a review from danix800 September 15, 2023 10:53
@danix800
Copy link
Member

LGTM! If no further comment I'll merge this in a day or two.

@mzyKi mzyKi force-pushed the bugfix/declrefexpr-structurally-equivalent branch from a1635d8 to 7e4a54f Compare September 21, 2023 06:53
@mzyKi mzyKi merged commit 3542168 into llvm:main Sep 21, 2023
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang][AST] fix lack comparison of declRefExpr in ASTStructuralEquivalence
4 participants