Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork33.7k
gh-104635: Add a test case for variables that have a dependency.#106583
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
corona10 commentedJul 10, 2023
I found this case while I worked for#106571 (comment), It will be helpful to the future researcher. |
| returna | ||
| self.assertEqual(f("x","y","z"),"y") | ||
| deftest_variable_dependent(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please explain the purpose of the test in a comment? From the code, it's nonibvious to me how it can be miscompiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you please explain the purpose of the test in a comment? From the code, it's nonibvious to me how it can be miscompiled.
No, it doesn't miscompile at this optimization moment.
But I was quite surprised with the CI never failed with the wrong compile for the#106571 (comment).
So it could be good to be added. I thought that miscompile case should be caught at the CI level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't say that the code is miscompiled. I propose adding a comment to explain that the test checks that this specific code is not miscompiled, but please explain why it could be miscompiled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done!
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM. I just suggest a minor phrasing change.
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: Carl Meyer <carl@oddbird.net>
vstinner left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
Uh oh!
There was an error while loading.Please reload this page.