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

Remove redundant access specifier from Phobos #5477

Merged
merged 1 commit into from
Oct 27, 2021

Conversation

wilzbach
Copy link
Member

Follow-up to #5466 (comment) and preparation for the Dscanner plugin: dlang-community/D-Scanner#441

std.unit seems to quite generous with scope-wide access specifiers. Hence, it might also make sense to reduce the impact of the almost module-wide private: ...

CC @andralex @DmitryOlshansky

@CyberShadow
Copy link
Member

Cool, but I thought the suggestion was

Oh, unindented private block. Thx. All blocks should be indented, @wilzbach how easy is this to enforce automatically?

@wilzbach
Copy link
Member Author

Cool, but I thought the suggestion was

Oh, unindented private block. Thx. All blocks should be indented, @wilzbach how easy is this to enforce automatically?

I should read messages on my phone in more detail 😆
Anyways checking for indents, isn't a big deal, see e.g.

dlang-community/D-Scanner#394

(I also opened a new Dscanner issue).

std/digest/md.d Outdated
@@ -179,7 +179,7 @@ struct MD5
S44 = 21,
}

private void transform(const(ubyte[64])* block) pure nothrow @nogc
Copy link
Member

Choose a reason for hiding this comment

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

I actually think there's merit in using private like this as opposed to the label form. In the label form the user has to browse in this case all the way up to line 98 to figure that the symbol is indeed private. And of course make sure there's no other intervening label. In the hierarchy of ease to assess, we have per-symbol (nicest), scope-introducing, and label.

Can you please change this PR a bit to only keep the label removals?

@wilzbach
Copy link
Member Author

Btw in case someone is interested, I have thrown together a PR for Dscanner that allows to use white and blacklists of modules for specific checks, so hopefully once this PR is merged, we don't need to go "all-or-nothing" in the beginning, but can gradually increase the propagation of a check.

@andralex
Copy link
Member

so what's the status here? on a cursory look lgtm

@wilzbach
Copy link
Member Author

so what's the status here? on a cursory look lgtm

I think I discovered a small bug in the RedundantAccessAttribute checker, that's why this on hold. I will get back to it in the next few days. Sorry.

On the bright side (thanks to #5495), we now have selective check for modules and everyone can help out reducing the check-specific blacklisted modules.

https://github.com/dlang/phobos/blob/master/.dscanner.ini

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @wilzbach!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#5477"

@RazvanN7
Copy link
Collaborator

@andralex I rebased this and should be good to go.

Copy link
Member

@andralex andralex left a comment

Choose a reason for hiding this comment

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

Fine, though I'm a fan of private attached to individual declaration (Java/C#/etc style) as opposed to long-acting private: which is brittle in the face of refactorings.

@RazvanN7 RazvanN7 merged commit 1f9e056 into dlang:master Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants