You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@geode.apache.org by Donal Evans <do...@vmware.com> on 2021/06/01 21:11:48 UTC
Re: Cleaning up the codebase - use curly braces everywhere
Given the positive response to this idea, I'm going to mark the PR as ready for review and contact the code owners required to get it merged. Thanks for all the feedback, and hopefully this can be the first step towards a concerted clean-up effort.
Donal
________________________________
From: Kirk Lund <kl...@apache.org>
Sent: Friday, May 28, 2021 3:19 PM
To: dev@geode.apache.org <de...@geode.apache.org>
Subject: Re: Cleaning up the codebase - use curly braces everywhere
+1 Also, I think if the entire work is done by IntelliJ then just mention
that in the PR description and I'm happy to add approval without stepping
through every line. I'd probably just pick a dozen random ones to check and
then approve it.
On Thu, May 27, 2021 at 6:36 PM Darrel Schneider <da...@vmware.com> wrote:
> +1 thanks for working on this
> ________________________________
> From: Donal Evans <do...@vmware.com>
> Sent: Thursday, May 27, 2021 10:22 AM
> To: dev@geode.apache.org <de...@geode.apache.org>
> Subject: Cleaning up the codebase - use curly braces everywhere
>
> Hi Geode dev,
>
> I've recently been looking at ways to improve code quality in small ways
> throughout the codebase, and as a starting point, I thought it would be
> good to make it so that we're consistently using curly braces for control
> flow statements everywhere, since this is something that's specifically
> called out in the Geode Code Style Guide wiki page[1] as one of the "more
> important points" of our code style.
>
> IntelliJ has a "Run inspection by name..." feature that makes it possible
> to identify all places where curly braces aren't used for control flow
> statements, (which showed over 3300 occurrences in the codebase) and also
> allows them to be automatically inserted, making the fix relatively
> trivial. Since this PR will touch 640 files, I wanted to make sure to first
> check that this is something even worth doing, and, if there's agreement
> that it is, to give reviewers context on what the changes are, the
> motivation for them, and how they were made, to help with the review
> process.
>
> The draft PR I have up[2] currently has no failing tests and can be marked
> as ready to review if there aren't any objections, and once it is, I'll try
> to coordinate with codeowners to get the minimal number of approvals
> required for a merge (it looks like only 6-7 reviewers are needed, though
> I'm sure that almost every code owner will be tagged as reviewers given the
> number of files touched).
>
> If this idea is a success, I think it would be good to have a discussion
> about other low-hanging code improvements we could make using static
> analysis (unnecessary casts, unused variables, duplicate conditions etc.),
> and, once a particular inspection has been "fixed," possibly consider
> adding a check for it as part of the PR pre-checkin to make sure it's not
> reintroduced. All thoughts and feedback are very welcome.
>
> Donal
>
> [1]
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FGEODE%2FCode%2BStyle%2BGuide&data=04%7C01%7Cdoevans%40vmware.com%7C6d413e51e7814e9999e308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=4An8MCpjP60dPIt%2BXbkmplHh6F8pc%2F0KdpTPO2KnvNI%3D&reserved=0
> [2]
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fgeode%2Fpull%2F6523&data=04%7C01%7Cdoevans%40vmware.com%7C6d413e51e7814e9999e308d92226b180%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637578371864383775%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=pufQ770JEdbJ%2FfWgT0rqQmrLJyFqQQjFc98Q%2FkYnHq4%3D&reserved=0
>