-
Notifications
You must be signed in to change notification settings - Fork 10.5k
ClangImporter: simplify logic for mutability attribute #61500
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
Conversation
@swift-ci please test Windows platform |
Take the spellings for the attributes as parameters for the diagnostics to allow us to reference the ignored the attribute properly. Use direct pointer checks instead of an unnecessary `llvm::Optional` around a nullable pointer. Use `llvm::StringRef` to simplify the comparison. This avoids re-spelling the attribute parameters which have already been checked by `isMutabilityAttr` for the contradiction.
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.
This reads better to me.
@swift-ci please test Windows platform |
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.
Err wait, it looks like the previous code would always override non mutating with mutating, so if you had both, it would resolve to mutating. Though I'm admittedly having some trouble following the original logic paths.
Okay, so |
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.
Okay, I'm more confident in this approval now. The original diagnostic appears to have been misleading since it only pointed at the non-mutating attribute, but didn't change anything else. The replacement patch doesn't affect the setting of seenMutabilityAttr
vs swiftAttr
compared the original code.
@swift-ci please smoke test |
@swift-ci please test |
CC: @zoecarver @egorzhdan |
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!
The Windows failure is due to Interop/Cxx/class/mutability-annotations-typechecker.swift which was addressed by #61512 |
Take the spellings for the attributes as parameters for the diagnostics to allow us to reference the ignored the attribute properly. Use direct pointer checks instead of an unnecessary
llvm::Optional
around a nullable pointer. Usellvm::StringRef
to simplify the comparison. This avoids re-spelling the attribute parameters which have already been checked byisMutabilityAttr
for the contradiction.