Fix redundant access-level modifiers. #1663
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5 tasks
|
@swift-ci please test |
|
Thank you for triggering the test, Simon! It would be greatly appreciated if you could also help review. |
|
How can we verify that we haven't accidentally changed the public visibility of some of these setters? |
|
@parkera Hi Tony, sorry I might not quite get what you meant. Can you point out some particular setters for example? Because I cannot see any existing property setter with Let us examine the relevant code. All extensions having their
In summary, all computed properties are get-only, and all static stored properties are constant. As a result, there isn't any property with a setter... When I was trying to remove the |
|
I guess I was looking for some kind of comparison of Swift's generated headers, as a check to make sure we aren't accidentally dropping public from something that had it before - or the opposite. Code inspection is good, but double checking is better. |
|
Hi Tony, do you mean and This is because their underlying serialised ASTs are different, though they result in the same access level effect for If we would have to perform some kind of automatic checking, an easy way I can come up with is to write some XCTest in When trying the approach above, I was stoped by something else. To begin with, I used Are you able to reproduce these errors? |
|
I don't think we need a unit test to verify all access, I just want to gain some additional confidence in this change. I'm not sure why the redundant access modifiers are a problem in the first place; it's often very useful as a library author to have a reminder of the public/private nature of what you're looking at. With them removed it lowers the confidence of a change being neutral to clients. |
|
I guess if we trust that the compiler is right about the warning, then perhaps just a list of the warnings from the change in your associated PR plus a list of the warnings from this change would be enough for now. |
|
Thanks for your suggestions, Tony! Actually, the modifications in this PR were inspired by the observation of warning lists. Here I collect the warning data for the following four scenarios:
Below is the comprehensive list of such 103 warnings: |
|
Great, thanks for double checking. This looks good to me. Can this be merged prior to your other change? |
parkera
approved these changes
Aug 27, 2018
|
Thank you, Tony! Yes, this one should be merged before landing the associated PR. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch aims at making corelibs-foundation adapted to this PR, which warns for redundant access-level modifiers.
@phausler Hi Philippe, would you help take a look at this, please?