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

Fixed lifetime issue in ast transform tests #17292

Merged

Conversation

lamarrr
Copy link
Contributor

@lamarrr lamarrr commented Nov 11, 2024

The ast operation takes references to the expressions. The expression was being copied to a lambda scope and destroyed at the end, leading to a dangling reference.
New code should use the ast tree for constructing complicated expression trees as it handles lifetime management.

Closes #17274

Description

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@lamarrr lamarrr requested a review from a team as a code owner November 11, 2024 22:26
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label Nov 11, 2024
@lamarrr lamarrr added bug Something isn't working non-breaking Non-breaking change labels Nov 11, 2024
@bdice
Copy link
Contributor

bdice commented Nov 12, 2024

New code should use the ast tree for constructing complicated expression trees as it handles lifetime management.

Would it be better to convert this test to use the new approach?

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Approving as a minimal fix, but we should evaluate refactoring the tests to use the new machinery that manages expression lifetimes.

Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

Verified this fixes the debug build gtest error.

@lamarrr
Copy link
Contributor Author

lamarrr commented Nov 13, 2024

New code should use the ast tree for constructing complicated expression trees as it handles lifetime management.

Would it be better to convert this test to use the new approach?

yes, that's the right thing to do. I'll do that in another merge request

@lamarrr
Copy link
Contributor Author

lamarrr commented Nov 13, 2024

/merge

@rapids-bot rapids-bot bot merged commit c4a4a91 into rapidsai:branch-24.12 Nov 13, 2024
107 checks passed
@lamarrr lamarrr deleted the transform-tests-lifetime-hotfix branch November 13, 2024 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] AST_TEST TransformTest.DeeplyNestedArithmeticLogicalExpression fails when run in a debug build
4 participants