-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Rework and fix associated const visibility for impl items #6785
base: master
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging #6785 will not alter performanceComparing Summary
|
12cbb05
to
c5036b0
Compare
c5036b0
to
24d0b1b
Compare
24d0b1b
to
083fbce
Compare
083fbce
to
6b9e216
Compare
6b9e216
to
5f5564b
Compare
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'm still confused by what visibility_level
means.
It also took me a while to figure out what problem this PR is supposed to solve in the first place. The PR deccription just links to an issue, but that issue only consists of an example program with no description of what the compiler does incorrectly for that program.
handler, | ||
engines, | ||
const_decl, | ||
Visibility::Public, |
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.
Am I right in thinking that const
s declared in traits are always public, whereas if they are declared anywhere else they are private unless they have the pub
modifier?
if is_self_type == IsSelfType::Yes { | ||
visibility_level = Visibility::Private; | ||
} | ||
|
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 suggest moving these lines down to just before we check for visibility_level.is_public()
. I'd also like a comment that explains why visibility_level
becomes private if resolve_symbol_and_mod_path
returns IsSelfType::Yes
.
@@ -317,7 +324,7 @@ pub fn resolve_call_path( | |||
} | |||
|
|||
// Otherwise, check the visibility modifier |
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.
Could you update this comment so that it explains what visibility_level
means? I'm still confused about it.
Alternatively explain it when visibility_level
is declared at the beginning of this function.
Description
Fixes #6772.
Checklist
Breaking*
orNew Feature
labels where relevant.