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

Prevent setting an undeclared attribute in Flags. #9543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sklam
Copy link
Member

@sklam sklam commented Apr 24, 2024

Prevent problem like this: #9539 (review)

@sklam sklam added the skip_release_notes Skip towncrier requirement label Apr 24, 2024
@sklam sklam marked this pull request as ready for review April 25, 2024 21:38
@sklam sklam added 3 - Ready for Review Effort - short Short size effort needed labels Apr 25, 2024
@gmarkall
Copy link
Member

gpuci run tests

@gmarkall
Copy link
Member

I have a strange recollection that this is somehow used for something somewhere, but I don't recall what - just running gpuCI in case the CUDA target is somehow overloading flags. It may be that a Flags instance gets created from a CUDAFlags at some point when overriding the target or something like that.

Copy link
Member

@gmarkall gmarkall left a comment

Choose a reason for hiding this comment

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

Looks good to me. I was concerned about this creating issues for target extensions, for example with my changes in #8773 and some follow-on changes I'd like to make, but I think we should merge this because it makes flag handling more robust, and if I run into an issue then I should propose a principled way to extend flags.

@gmarkall
Copy link
Member

I would mark this RTM but I don't want to cause any confusion before before the branch for 0.60, so I'll mark it RTM once we've branched.

@gmarkall gmarkall added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 3 - Ready for Review labels Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4 - Waiting on reviewer Waiting for reviewer to respond to author Effort - short Short size effort needed skip_release_notes Skip towncrier requirement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants